[Target] Automatically detect system triple when not specified by the user #16513
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lhutton1
force-pushed
the
detect-native-triple
branch
2 times, most recently
from
02f6b51
to
4d27827
Compare
|
cc @ekalda @neildhickey @cbalint13 @kparzysz-quic would be good to hear your thoughts before I proceed with creating the issues for the failing tests (due to test coverage in CI increasing). |
cbalint13
approved these changes
Feb 6, 2024
cbalint13
reviewed
Feb 6, 2024
This was referenced Feb 7, 2024
Open
lhutton1
force-pushed
the
detect-native-triple
branch
2 times, most recently
from
25564e3
to
61795d9
Compare
… user Currently, when a default compile target such as llvm is specified, it implies llvm -keys=cpu which tends to imply x86 related components being used during compilation e.g. the schedules registered in TOPI. This can be confusing for a user when compiling on other architectures, especially when other tools such as llc infer the default target based on the host. When the target kind is llvm, this commit uses the "target.llvm_get_system_triple" functionality to automatically detect mtriple when one has not been provided in the target string. The target will be updated to one that uses the mtriple of the host: llvm -> llvm -mtriple=<system-triple>. When compiling on Arm(R)-based targets, this has the added benfit of automatially introducing -keys=arm_cpu to the target improving the schedule selection. Lots of tests are currently using targets such as llvm or similar which has resulted in a lack of coverage of other targets such as arm_cpu. As part of this commit, failing test cases which have simple / obvious issues have been fixed. Others that likely need more thought have been skipped. In doing so, it reduces the number of modifications and simplifies the review for this change. Note: This PR is marked as draft while checking and fixing other failures in CI. Tests marked as skipped containing "<Issue link>" in the reason will be have issues added and the related reason will be updated when CI is green. This commit is a follow up of the changes made in: apache#14981 Change-Id: Icee7f5c00d58fc77367c823273fccae128260471 Co-authored-by: Jack Frankland <jack.frankland@arm.com>
Change-Id: Ic1eff47b7cdb9170f71d646461db51cb26e14881
Change-Id: Ibc67e73e8dcfb327dc976a92f12253738d6ad179
Change-Id: I4be5c8f64850a3612516c0b49ec8fcf4191a4fbb
Change-Id: Ia79d879399ad7f2d098fd4a0af5c29a89565133e
Change-Id: Ie4e91a9b45fe6a8b22d6faed3334290b93ab27bc
lhutton1
force-pushed
the
detect-native-triple
branch
from
61795d9
to
f85803f
Compare
|
@tvm-bot rerun |
ekalda
approved these changes
Mar 13, 2024
There was a problem hiding this comment.
Thanks @lhutton1 and @FranklandJack for looking into this! The patch has been open for a bit, so let's see if CI is still happy...
|
Thanks @lhutton1 @FranklandJack @cbalint13! |
thaisacs
pushed a commit
to thaisacs/tvm
that referenced
this pull request
Apr 3, 2024
… user (apache#16513) Currently, when a default compile target such as llvm is specified, it implies llvm -keys=cpu which tends to imply x86 related components being used during compilation e.g. the schedules registered in TOPI. This can be confusing for a user when compiling on other architectures, especially when other tools such as llc infer the default target based on the host. When the target kind is llvm, this commit uses the "target.llvm_get_system_triple" functionality to automatically detect mtriple when one has not been provided in the target string. The target will be updated to one that uses the mtriple of the host: llvm -> llvm -mtriple=<system-triple>. When compiling on Arm(R)-based targets, this has the added benfit of automatially introducing -keys=arm_cpu to the target improving the schedule selection. Lots of tests are currently using targets such as llvm or similar which has resulted in a lack of coverage of other targets such as arm_cpu. As part of this commit, failing test cases which have simple / obvious issues have been fixed. Others that likely need more thought have been skipped. In doing so, it reduces the number of modifications and simplifies the review for this change. This commit is a follow up of the changes made in: apache#14981 Change-Id: Icee7f5c00d58fc77367c823273fccae128260471 Co-authored-by: Jack Frankland <jack.frankland@arm.com> --------- Co-authored-by: Jack Frankland <jack.frankland@arm.com>
lhutton1
added a commit
that referenced
this pull request
Apr 11, 2024
…pu.cc DetectSystemTriple() (#16766) I ran into a problem running a very simple ONNX compile, i would get a segfault at a FoldConstantExpr() call from TVMC. **This only happens if the compile flag `set(USE_LLVM OFF)` is OFF.** ``` Thread 1 "python3" received signal SIGSEGV, Segmentation fault. 0x00007fffc94ac78c in tvm::runtime::ObjectPtr<tvm::runtime::Object>::operator!=(decltype(nullptr)) const (this=0x0, null=<error reading variable: Attempt to dereference a generic pointer.>) at /home/otto/tvm/include/tvm/runtime/object.h:470 470 bool operator!=(std::nullptr_t null) const { return data_ != null; } ``` I had compiled TVM Using GCC: ``` COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/11/lto-wrapper OFFLOAD_TARGET_NAMES=nvptx-none:amdgcn-amdhsa OFFLOAD_TARGET_DEFAULT=1 Target: x86_64-linux-gnu Configured with: ../src/configure -v --with-pkgversion='Ubuntu 11.4.0-1ubuntu1~22.04' --with-bugurl=file:///usr/share/doc/gcc-11/README.Bugs --enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,m2 --prefix=/usr --with-gcc-major-version-only --program-suffix=-11 --program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib --enable-libphobos-checking=release --with-target-system-zlib=auto --enable-objc-gc=auto --enable-multiarch --disable-werror --enable-cet --with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32 --enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-nvptx/usr,amdgcn-amdhsa=/build/gcc-11-XeT9lY/gcc-11-11.4.0/debian/tmp-gcn/usr --without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu --with-build-config=bootstrap-lto-lean --enable-link-serialization=2 Thread model: posix Supported LTO compression algorithms: zlib zstd gcc version 11.4.0 (Ubuntu 11.4.0-1ubuntu1~22.04) ``` This was caused by a call to defined() from DetectSystemTriple() in cpu.cc that was added in #16513. When the previous call ``` auto pf = tvm::runtime::Registry::Get("target.llvm_get_system_triple"); ``` would fail, and return null. The consecutive call to defined() would segfault after trying to dereference the null value. This commit adds a check to see if the function pointer is null. This might not be the best solution, but it worked for me, so it might also help someone else struggling with this. Please suggest a better solution, if you know one. Co-authored-by: Luke Hutton <luke.hutton@arm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, when a default compile target such as
llvmis specified, it impliesllvm -keys=cpuwhich tends to imply x86 related components being used during compilation e.g. the schedules registered in TOPI. This can be confusing for a user when compiling on other architectures, especially when other tools such asllcinfer the default target based on the host.When the target kind is
llvm, this commit uses the "target.llvm_get_system_triple" functionality to automatically detectmtriplewhen one has not been provided in the target string. The target will be updated to one that uses themtripleof the host:llvm -> llvm -mtriple=<system-triple>. When compiling on Arm(R)-based targets, this has the added benefit of automatically introducing-keys=arm_cputo the target improving the schedule selection.Lots of tests are currently using targets such as
llvmor similar which has resulted in a lack of coverage of other targets such asarm_cpu. As part of this commit, failing test cases which have simple / obvious issues have been fixed. Others that likely need more thought have been skipped. In doing so, it reduces the number of modifications and simplifies the review for this change.Note:
This PR is marked as draft while checking and fixing other failures in CI. Tests marked as skipped containing "issue link" in the reason will have issues added and the related reason will be updated when CI is green.This commit is a follow up of the changes made in: #14981
Co-authored-by: Jack Frankland jack.frankland@arm.com