[Driver][Offloader] Add getOffloadingDeviceToolChain function #176
Conversation
getOffloadingDeviceToolChain is the equivalent of getToolChain for device ToolChains. It may be possible to merge the two but as Offloading Devices require some extra information (like host/device triple and OffloadKind) it may get a little messy. The switch statement in getToolChain also already seems a little complex. So it may not be worth merging and over complicating them. In general using getOffloadingDeviceToolChain moves some of the shared logic for device ToolChain selection for SYCL/CUDA/HIP/OpenMP into the function call, so it refactors it a bit. But in a more specific application to the SYCL implementation this call allows a ToolChain implementer to add their own ToolChain in place of the SYCL ToolChain when targeting a specific host / device triple. However, if they did not want to add their own ToolChain and simply wanted to add a new tool to the existing SYCL ToolChain they'd still be able to do that via overloading/adding to the GetTools method inside of the SYCLToolChain. So the intent is just to add more flexibility, rather than replace some functionality with some other functionality. Signed-off-by: Andrew Gozillon <andrew.gozillon@yahoo.com>
agozillon
force-pushed
the
getOffloadingDevice
branch
from
c7e34ed to
112d238
Compare
…YCL target No longer will the switch statement assert with llvm_unreachable, instead the appropriate Clang diagnostic warning for an invalid SYCL output should be emitted. This occurs in the initialize() call for the SYCL offloader/phase and action generator, similar to how CUDA emits errors for incorrect GPU Arch targets. Doing it similarly to OpenMP (inside CompilerInvocation.cpp) doesn't work in this case as if we pass an invalid target we won't accidentally generate an invalid/incorrect ToolChain like GetToolChain's we will instead return a nullptr which will ICE the compiler before it reaches CompilerInvocation.cpp, so the warning interaction has to be slightly different in this case. Also removing some unused variables I missed in the first commit. Signed-off-by: Andrew Gozillon <andrew.gozillon@yahoo.com>
|
Should correctly output error diagnostics rather than assert via llvm_unreachable now. I initially tried to do it it similarly to OpenMP (inside CompilerInvocation.cpp) however, it doesn't work in this case as if we pass an invalid target we won't accidentally generate an invalid/incorrect ToolChain like getToolChain does in most cases, we will instead return a nullptr which will ICE the compiler before it reaches the relevant location in CompilerInvocation.cpp, so the error diagnostics has to be preemptive in this case. Note: This is only a problem for SYCL as it's the only offloading programming model at the moment that allows multiple non-hardcoded device offload targets via the getOffloadingDeviceToolChain call. HIP, OpenMP and CUDA should not run into the same issue, they should always generate the same appropriate ToolChain at the moment. The way the diagnostic works in the current commit is somewhat similar to the way CUDA checks for invalid GPU Archs in its own Initialize (https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L2594). Although, I'd be more inclined to modify the current commit to check for it here: https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L780 alongside the existing unknown target check, it would deviate a little from how it's done for CUDA/OpenMP but to me it makes more sense to emit the diagnostic as early as possible (before its selected an incorrect ToolChain) and in the same location as the other err_drv_invalid_sycl_target diagnostic. I'm fine with either approach though (or an alternative), so I'll leave the decision in the reviewers hands. |
|
@mdtoguchi, are you okay to merge this version or do you want @agozillon to apply changes he proposed here #176 (comment)? Or maybe you have an alternative proposal? |
|
I like the idea of adding the check here: https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L780. The generic Unknown check is pretty broad and doesn't really do anything for us. |
This should be an NFC. Moved the diagnostic from the OffloadingActionBuilder's initialize method to the Driver's CreateOffloadingDeviceToolChains call. No need to track it in two places, invalid target caught as early as required. Should be no chance of ICE'ng the compiler by accessing a non-existant ToolChain. Signed-off-by: Andrew Gozillon <andrew.gozillon@yahoo.com>
|
Moved the target check from the initialize method to https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L780 in the last commit. |
New test case aims to showcase that existing architecture triples that are not valid for SYCL offloading also get rejected. In this case, just testing using x86_64. Signed-off-by: Andrew Gozillon <andrew.gozillon@yahoo.com>
This pull request is in relation to: #53 where I was wondering, what the best way (or the way you guys would like us to) to integrate other ToolChains is and it was discussed a little during one of the Upstreaming meetings.
Currently in our derivation of your implementation I've added this getOffloadingDeviceToolChain method which allows you to just add a new offloading device ToolChain for selection to the switch statement based on the passed in triple. It's reflective of getToolChain (https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L4974), except for offloading device ToolChains. You can see a rudimentary example of us adding a new ToolChain here: https://github.com/triSYCL/sycl/blob/sycl/unified/master/clang/lib/Driver/Driver.cpp#L5142
Nothing too complex, but it allows the addition of new ToolChain's very easily for new Triples and follows a similar concept to OpenMP's usage of getToolChain (although it does work differently when it's using NVPTX): https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/Driver.cpp#L705
If implementers trying to target new devices with new tools don't wish to add a new ToolChain it's still completely possible to just add new Tools to the default SYCL ToolChain by overloading (or adding to) the SelectTool method and adding their Tools to the SYCL ToolChain similar to the Myriad ToolChain: https://github.com/intel/llvm/blob/sycl/clang/lib/Driver/ToolChains/Myriad.cpp#L265
I decided on a separate function in this case rather than merging the behavior of getToolChain and getOffloadingDeviceToolChain as offloading devices function a little differently (indexed by Host / Device triple and at least in CUDA's case require Action::OffloadKind)
I think this method allows people to easily add new ToolChains rather than feel the need to add it all into the existing SYCL ToolChain as new Tools and alter the existing logic of the ToolChain (perhaps that's the intent though, but I imagine a single ToolChain approach could end up quite complex if it extends to a lot of architectures, I could be wrong however). It also has the minor benefit of refactoring some of the repeated logic for creating device ToolChains into a function.
Perhaps this isn't how everyone would like new ToolChains added however, or someone with more experience with the Driver (in particular the phases/action building) can see a problem in supporting ToolChains this way? Or maybe there is another direction that's better? Either-way happy for feedback! Also happy to answer any questions, especially if my description is unclear in any way.
The commits log is a (hopefully) slightly more concise explanation of the above if more context is required!
Edit: Force pushed to add sign off