Page MenuHomePhabricator

[OpenMP] Extend CLANG target options with device offloading kind.
ClosedPublic

Authored by gtbercea on Feb 7 2017, 8:19 AM.

Details

Summary

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.

Event Timeline

gtbercea created this revision.Feb 7 2017, 8:19 AM
gtbercea added a subscriber: cfe-commits.
jlebar added inline comments.Feb 7 2017, 4:53 PM
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?

gtbercea updated this revision to Diff 93181.Mar 27 2017, 3:05 PM
gtbercea marked 5 inline comments as done.

Update patch to reflect latest source code changes.

gtbercea added inline comments.Mar 27 2017, 3:06 PM
lib/Driver/ToolChains.cpp
4902 ↗(On Diff #87440)

Agreed. I've changed it to reflect that, currently, only OpenMP and CUDA offloading kinds are supported for this toolchain.

4914 ↗(On Diff #87440)

Reverted them, since they only concern line length.

4961 ↗(On Diff #87440)

Changed.

gtbercea added inline comments.Mar 27 2017, 3:08 PM
lib/Driver/Tools.cpp
12136 ↗(On Diff #87440)

I have to investigate this. Thanks for pointing this out.

Hahnfeld edited edge metadata.May 4 2017, 10:39 AM

One minor drive-by comment. I think there is still one outstanding from Justin...

lib/Driver/ToolChains/Cuda.cpp
347

You can move the declaration down to the initialization

hfinkel edited edge metadata.Jun 21 2017, 6:22 PM

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?

412

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?

gtbercea marked an inline comment as done.Jun 28 2017, 7:29 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
412

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.

hfinkel added inline comments.Jun 28 2017, 7:34 AM
lib/Driver/ToolChains/Cuda.cpp
412

Which mailing list thread discussed this?

gtbercea marked an inline comment as done.Jun 28 2017, 7:41 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
412

I believe it was openmp-dev. The proposal was initially posted by Gregory Rodgers.

hfinkel added inline comments.Jun 28 2017, 7:51 AM
lib/Driver/ToolChains/Cuda.cpp
412

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.

gtbercea added inline comments.Jun 28 2017, 8:02 AM
lib/Driver/ToolChains/Cuda.cpp
412

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. :-(

gtbercea updated this revision to Diff 104427.Jun 28 2017, 8:34 AM

Updated diff to address comments.

gtbercea marked an inline comment as done.Jun 28 2017, 8:37 AM
gtbercea added inline comments.
lib/Driver/ToolChains/Cuda.cpp
217

Just added a test case for this.

347

Done! Thanks for spotting that.

gtbercea marked an inline comment as done.Jun 28 2017, 9:09 AM
hfinkel added inline comments.Jun 28 2017, 9:30 AM
test/Driver/openmp-offload.c
614 ↗(On Diff #104427)

How does this work on x86 where the host also uses -march?

gtbercea added inline comments.Jun 28 2017, 11:19 AM
test/Driver/openmp-offload.c
614 ↗(On Diff #104427)

That's where the additional flag comes in. The new flag *should* be used instead.

hfinkel added inline comments.Jun 28 2017, 11:33 AM
test/Driver/openmp-offload.c
614 ↗(On Diff #104427)

Ah, okay. Please just propose a patch which adds a new flag. This workaround is a bit strong for my tastes.

gtbercea added inline comments.Jun 28 2017, 11:38 AM
test/Driver/openmp-offload.c
614 ↗(On Diff #104427)

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.

gtbercea updated this revision to Diff 104527.EditedJun 28 2017, 3:27 PM

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.

hfinkel accepted this revision.Jun 29 2017, 8:15 AM

LGTM

This revision is now accepted and ready to land.Jun 29 2017, 8:15 AM

LGTM

When you commit this, please make sure to mention in the commit message that the test cases will be associated with follow-up commits.

gtbercea closed this revision.Jul 6 2017, 9:22 AM