-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL][Driver][sycl-xocc] Experimental SYCL Driver Patch, More Aligned With OpenMP and xocc script/ToolChain refactoring #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First batch
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 |
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!
c3f77f9
to
352e79d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks good.
Just a few remarks.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it a bug or forgotten?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
93b9b97
to
2e741e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I have just noticed some alignment issues.
sycl/test/xocc_tests/sdaccel_ports/vision/edge_detection/edge_detection.cpp
Outdated
Show resolved
Hide resolved
Great! |
CONFLICT (content): Merge conflict in clang/include/clang/Basic/DiagnosticSemaKinds.td
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
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).