Pass the type of the device offloading when building the tool chain for a particular target architecture. This is required when supporting multiple tool chains that target a single device type. In our particular use case, the OpenMP and CUDA tool chains will use the same `addClangTargetOptions ` method. This enables the reuse of common options and ensures control over options only supported by a particular tool chain.
Details
- Reviewers
arpith-jacob caomhin carlo.bertolli ABataev jlebar hfinkel tstellar Hahnfeld - Commits
- rGf0f29608d052: [OpenMP] Extend CLANG target options with device offloading kind.
rC307272: [OpenMP] Extend CLANG target options with device offloading kind.
rL307272: [OpenMP] Extend CLANG target options with device offloading kind.
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Driver/ToolChains.cpp | ||
---|---|---|
4902 ↗ | (On Diff #87440) | Not sure this assertion message helps us much beyond what's already in the code. |
4914 ↗ | (On Diff #87440) | Are these changes related to this patch? I have no problem cleaning up whitespace errors like these, but would prefer for them to be split out separately if possible. |
4961 ↗ | (On Diff #87440) | s/device/compilation/? |
4961 ↗ | (On Diff #87440) | An "otherwise" would probably be helpful in this comment. |
4966 ↗ | (On Diff #87440) | TODO is idiomatically one word |
lib/Driver/Tools.cpp | ||
12136 ↗ | (On Diff #87440) | Why does JA.getOffloadingArch have the wrong value? Isn't the purpose of JA.getOffloadingArch to have this particular value? If it's broken, can we fix it instead of doing this hack? |
lib/Driver/Tools.cpp | ||
---|---|---|
12136 ↗ | (On Diff #87440) | I have to investigate this. Thanks for pointing this out. |
One minor drive-by comment. I think there is still one outstanding from Justin...
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
368 | You can move the declaration down to the initialization |
In general, this patch seems to be missing tests (unless it is actually NFC, or you can't write tests yet, which, in either case, need to be explained).
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
217 | Do we have a test case for this? | |
432 | Why is this a TODO? Is the necessary information not currently available in the offloading arguments, or are we just not grabbing it in this patch? |
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
432 | This is for a future patch in which we will grab the compute capability from a special flag. I believe there has already been some discussion on the dev mailing list as to what that flag should be but I don't think there was a general consensus as to how that flag should be named. |
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
432 | Which mailing list thread discussed this? |
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
432 | I believe it was openmp-dev. The proposal was initially posted by Gregory Rodgers. |
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
432 | Can you please find it and post the link from here: http://lists.llvm.org/pipermail/openmp-dev/ I do recall discussing this at some point somewhere, but I'm not finding it in my email. |
lib/Driver/ToolChains/Cuda.cpp | ||
---|---|---|
432 | I managed to find the e-mail at last. It looks like it was a side conversation which ended with Justin Lebar suggesting Greg to post the discussion on the llvm-dev list. That post never happened. :-( |
test/Driver/openmp-offload.c | ||
---|---|---|
614 | How does this work on x86 where the host also uses -march? |
test/Driver/openmp-offload.c | ||
---|---|---|
614 | That's where the additional flag comes in. The new flag *should* be used instead. |
test/Driver/openmp-offload.c | ||
---|---|---|
614 | Ah, okay. Please just propose a patch which adds a new flag. This workaround is a bit strong for my tastes. |
test/Driver/openmp-offload.c | ||
---|---|---|
614 | Completely agree! It's not a solution I am happy with it either as temporary as it is. I will propose a patch with a new flag. |
Split previous diff into a "device offloading kind" patch (show here) and a new patch D34784 which relies on a new compiler flag.
A TODO has been added to signal that the compute capability is to be handled in the new patch mentioned above.
When you commit this, please make sure to mention in the commit message that the test cases will be associated with follow-up commits.
Do we have a test case for this?