[SYCL][Driver][sycl-xocc] Experimental SYCL Driver Patch, More Aligned With OpenMP and xocc script/ToolChain refactoring #24
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
|
I marked as resolved the things I believe I fixed in the previous commit, so that it's easier to see what may need further discussion or might not be feasible in this patch Note: Force pushed a failed rebase attempt that ruined the history and then second force push to correctly rebase on master |
agozillon
force-pushed
the
sycl/unified/driver_experiment
branch
from
beceb01
to
c3f77f9
Compare
Need to instantiate the linker and assembler slightly differently now that we're no longer using the internal GetTools call.
…e argument Unfortunately, while the user is no longer required to specify a command line argument to the user, the compiler still needs a new Option for information to be pushed from the Driver onto both the host and device for now. If it's just device related, it would be feasible to check the Target triple. Sadly the host invocation doesn't contain any knowledge of the offloaded triple from what I can tell (aux-triple would be nice to use, but it seems this is largelly used to pass host triple information to device offloading passes and not vice-versa). Changed -fsycl-xocc-device -> -fsycl-xocc, as it currently enforces both host and device related tweaks (mainly forces the host definition of __SYCL_XILINX_ONLY__, which I think should be refactored out long term for a runtime solution if feasible). Moved option -fsycl-xocc from Options.td to CC1Options.td, as it's more "Clang" specific rather than part of the driver frontend at the moment (shouldn't really be read by the driver/isn't at the moment). Perhaps, a bit pedantic... Adding a test to check internal compiler defines that should print some messages based on some defines that should be generated by the compiler.
Should be an NFC, but simplifies and cleans up the implementation a little.
This shouldn't change any existing functionallity, it is cleaning the driver by: * Moving construct commands from the XOCC Tool Chain into the sycl-xocc shell script, resulting in 1 single command for the Tool Chain to the script * Combining the XOCC Compiler and Linker Command phases into one, in both the Script and commands Overall this should result in a much easier to read and understand set of tools to build on.
* Fixed documentation and tests to use new fsycl-targets * Added RUN: true to the top of new test so that it passes by default with check-all * Removed some old todo's from XOCC.h
…oadKind I think this makes more snese than the previous version that used the Arch > OS categorization method like GetToolChain. Open to changing it back though or altering it to a different categorization method!
agozillon
force-pushed
the
sycl/unified/driver_experiment
branch
from
c3f77f9
to
352e79d
Compare
keryell
reviewed
May 3, 2019
| @@ -211,7 +214,7 @@ class Triple { | |||
| CoreCLR, | |||
| Simulator, // Simulator variants of other systems, e.g., Apple's iOS | |||
| SYCLDevice, | |||
| LastEnvironmentType = Simulator | |||
| LastEnvironmentType = SYCLDevice | |||
There was a problem hiding this comment.
I think when they added the SYCLDevice triple they forgot to update the LastEnvironmentType, so it appears to be a something that was forgotten when added. Last_Type should always point to the last element.
In either case, I haven't encountered any actual problems with the LastEnvironmentType correctly or incorrectly pointing to the final enum element yet. But worth correcting incase something does occur and confuse someone eventually.
| LLVM_LINK="$2/llvm-link" | ||
| XOCC="$1/xocc" | ||
|
|
||
| $OPT -O3 -asfix -globaldce -inSPIRation -globaldce -kernelNameGen "$4" -o \ |
There was a problem hiding this comment.
Should the -O3 come from a compiler option instead?
And how to provide -g or whatever other option?
There was a problem hiding this comment.
I think this should come in a separate patch, there are a lot of things desirable to be passed through to the script e.g. XILINX_PLATFORM and XCL_EMULATION_MODE would be better off as arguments than environment variables. However, at the bare minimum it requires parsing and then passing through more command line arguments via the XOCC ToolChain as well as some script modifications. It would also be interesting to look at some of the other possible compiler options we could filter down.
Perhaps a little outside of the scope of this patch? And can be opened up as a new issue for a future patch?
| [ $# -eq 0 ] && usage 2 "no compilation option specified" | ||
| [ $# -eq 0 ] && usage 3 "no data file" | ||
| if [[ -z "$1" ]]; then | ||
| usage 1 "no sdx bin directory containing xocc" |
There was a problem hiding this comment.
SDx?
I have the feeling that bin is an implementation of current SDx install ad should not me mentioned here and in the following.
There was a problem hiding this comment.
I'm not too sure I understand what you mean, what would you prefer instead?
|
Updated and resolved the review points I think I have corrected, the rest are open for more discussion if required |
Largelly just tidying up the new test and documentation, nothing important.
agozillon
force-pushed
the
sycl/unified/driver_experiment
branch
from
93b9b97
to
2e741e2
Compare
agozillon
changed the title
agozillon
changed the title
agozillon
changed the title
agozillon
changed the title
keryell
reviewed
May 8, 2019
sycl/test/xocc_tests/sdaccel_ports/vision/edge_detection/edge_detection.cpp
Outdated
Show resolved
Hide resolved
|
Great! |
Ralender
pushed a commit
to Ralender/sycl
that referenced
this pull request
Jul 1, 2020
CONFLICT (content): Merge conflict in clang/include/clang/Basic/DiagnosticSemaKinds.td
keryell
pushed a commit
that referenced
this pull request
Jan 4, 2023
Found by msan -fsanitize-memory-use-after-dtor.
==8259==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x55dbec54d2b8 in dtorRecord(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:150:22
#1 0x55dbec54bfcf in dtorArrayDesc(clang::interp::Block*, char*, clang::interp::Descriptor*) clang/lib/AST/Interp/Descriptor.cpp:97:7
#2 0x55dbec508578 in invokeDtor clang/lib/AST/Interp/InterpBlock.h:79:7
#3 0x55dbec508578 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:55:19
#4 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
#5 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
#6 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
#7 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
#8 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
#9 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
#10 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
#11 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
#12 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
#13 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
#14 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
#15 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
#16 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
#17 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
#18 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
#19 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
#20 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
#21 0x7f9be07fa632 in __libc_start_main
#22 0x55dbe6a702e9 in _start
Member fields were destroyed
#0 0x55dbe6a7da5d in __sanitizer_dtor_callback_fields compiler-rt/lib/msan/msan_interceptors.cpp:949:5
#1 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:479:7
#2 0x55dbec5094ac in ~SmallVectorImpl llvm/include/llvm/ADT/SmallVector.h:612:3
#3 0x55dbec5094ac in llvm::SmallVector<clang::interp::Record::Base, 8u>::~SmallVector() llvm/include/llvm/ADT/SmallVector.h:1207:3
#4 0x55dbec508e79 in clang::interp::Record::~Record() clang/lib/AST/Interp/Record.h:24:7
#5 0x55dbec508612 in clang::interp::Program::~Program() clang/lib/AST/Interp/Program.h:49:26
#6 0x55dbec50657a in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
#7 0x55dbec50657a in std::__msan::unique_ptr<clang::interp::Program, std::__msan::default_delete<clang::interp::Program>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
#8 0x55dbec5035a1 in clang::interp::Context::~Context() clang/lib/AST/Interp/Context.cpp:27:22
#9 0x55dbebec1daa in operator() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:55:5
#10 0x55dbebec1daa in std::__msan::unique_ptr<clang::interp::Context, std::__msan::default_delete<clang::interp::Context>>::~unique_ptr() third_party/crosstool/v18/stable/toolchain/bin/../include/c++/v1/__memory/unique_ptr.h:261:7
#11 0x55dbebe285f9 in clang::ASTContext::~ASTContext() clang/lib/AST/ASTContext.cpp:1038:40
#12 0x55dbe941ff13 in llvm::RefCountedBase<clang::ASTContext>::Release() const llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:101:7
#13 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:159:38
#14 0x55dbe94353ef in release llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:224:7
#15 0x55dbe94353ef in ~IntrusiveRefCntPtr llvm/include/llvm/ADT/IntrusiveRefCntPtr.h:191:27
#16 0x55dbe94353ef in clang::CompilerInstance::setASTContext(clang::ASTContext*) clang/lib/Frontend/CompilerInstance.cpp:178:3
#17 0x55dbe95ad0ad in clang::FrontendAction::EndSourceFile() clang/lib/Frontend/FrontendAction.cpp:1100:8
#18 0x55dbe9445fcf in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) clang/lib/Frontend/CompilerInstance.cpp:1047:11
#19 0x55dbe6b3afef in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:266:25
#20 0x55dbe6b13288 in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) clang/tools/driver/cc1_main.cpp:250:15
#21 0x55dbe6b0095f in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) clang/tools/driver/driver.cpp:319:12
#22 0x55dbe6aff41c in clang_main(int, char**) clang/tools/driver/driver.cpp:395:12
#23 0x7f9be07fa632 in __libc_start_main
#24 0x55dbe6a702e9 in _start
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.
This patch is related to the issue/discussion in Intel/SYCL: intel/llvm#53 but also contains a significant amount of refactoring of the xocc driver implementation.
For example, it also includes significant refactoring of the xocc ToolChain and sycl-xocc script. It largely squashes the linker and assembler stages in the script and ToolChain into one linker phase and moves ConstructJob calls from the ToolChain into the script making the script the main way to alter what occurs when building, the Toolchain's only real job is to find Xilinx tools and create a call to the script for now.
The patch also adds an FPGA32/64 arch triple component and Xilinx vendor triple component so that we can force the compiler to use our ToolChain using fsycl-target rather than using -fsycl-xocc-device
The changes relating to issue 53 on the Intel SYCL repository are mainly the addition of a getOffloadingDeviceToolChain function inside the Clang Driver, which replaces hardcoded ToolChain calls/creation for CUDA/SYCL/OpenMP+NVPTX/HIP in favor of something similar to the getToolChain call that OpenMP (without NVPTX) uses. This way future users can have a separate ToolChain from the SYCL ToolChain and still have a way to redirect to it if they wish. This moves the requirement to have all device Tools redirected to and contained inside the SYCL ToolChain, making it the defacto master chain which could get a little crowded overtime. They can also have there own overload rules without having to alter the existing SYCL ToolChain overloads e.g. having to optionally specify an assembler stage based on target. However, overloading the SelectTool method inside of the SYCL ToolChain is still an option if an implementer really wants to just add new optional Tools to the existing SYCL ToolChain like we did prior to this patch.
So, in essence it should give the implementer the freedom to choose if they wish to add a completely new Offload Device ToolChain via getOffloadingDeviceToolChain (OpenMP style) or overload the SelectTool call and add additional Tools to the existing default SYCL ToolChain (MyriadToolChain style).