This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Consistently use cubin extension for nvlink
ClosedPublic

Authored by Hahnfeld on Nov 20 2017, 7:08 AM.

Details

Summary

This was previously done in some places, but for example not for
bundling so that single object compilation with -c failed. In
addition cubin was used for all file types during unbundling which
is incorrect for assembly files that are passed to ptxas.
Tighten up the tests so that we can't regress in that area.

Diff Detail

Event Timeline

Hahnfeld created this revision.Nov 20 2017, 7:08 AM

A different approach would be to completely replace the type in Driver::ConstructPhaseAction but

  1. this was previously considered a too radical change,
  2. we currently don't have the necessary information (ToolChain, OffloadKind) to take that decision,
  3. this might break in subtle places that compare the type with TY_Object.
gtbercea added inline comments.Nov 20 2017, 8:46 AM
lib/Driver/ToolChains/Clang.cpp
5340

Please add a comment here describing what this entire code snippet is doing.

lib/Driver/ToolChains/Cuda.cpp
431

Is this always a cubin?

461

When does this situation occur?

Hahnfeld marked 2 inline comments as done.Nov 20 2017, 8:51 AM
Hahnfeld added inline comments.
lib/Driver/ToolChains/Cuda.cpp
431

Probably because the linker always takes object files...

461

Well, if that condition fires:

  1. For CUDA
  2. For other types. For example, the bundler might need to bundle / unbundle assembly files.
gtbercea added inline comments.Nov 20 2017, 9:01 AM
lib/Driver/ToolChains/Cuda.cpp
431

Considering that the function may also return an object file can you add an assert here that getInputFileName(II) returns a cubin in this case?

461

Can you put this info in a comment just before the if statement?

test/Driver/openmp-offload-gpu.c
51

This is kind of optional but since the name of the tool contains the word "bundler" and you are performing an unbundling operation, could you also check that the -unbundle flag is passed?

Hahnfeld marked 2 inline comments as done.Nov 20 2017, 9:07 AM
Hahnfeld added inline comments.
lib/Driver/ToolChains/Cuda.cpp
431

Hmm, that's a string. I've never seen asserts that say "this string should contain...". IMO that's what we have tests for

461

I'd say this condition is not the most difficult that I've ever seen, but can do

gtbercea added inline comments.Nov 20 2017, 10:31 AM
lib/Driver/ToolChains/Cuda.cpp
431

Ok no problem then let's leave it up to the test :)

461

Thanks!

tra added a subscriber: tra.Nov 20 2017, 10:39 AM
tra added inline comments.
lib/Driver/ToolChains/Clang.cpp
5341–5344

Can we ever have more than one dependence? If not, perhaps add an assert.
Otherwise, can dependencies have different toolchains?
If so, which one do we really want?

lib/Driver/ToolChains/Cuda.cpp
461

I usually find conditions expressed in positive terms are much easier to understand at a glance.
I.e. if (!(OK == Action::OFK_OpenMP && Input.getType() == types::TY_Object)) is, IMO, almost obvious in its meaning, while the condition above required conscious effort to understand its purpose.

lib/Driver/ToolChains/Cuda.h
144

virtual is redundant here.

Hahnfeld updated this revision to Diff 123649.Nov 20 2017, 1:41 PM
Hahnfeld marked 13 inline comments as done.

Address reviewers' comments.

lib/Driver/ToolChains/Clang.cpp
5341–5344

No, there should only be one for inputs to the bundler. I've added the assert and all tests pass.

lib/Driver/ToolChains/Cuda.h
144

(I've also changed this for getAuxTriple above.)

tra added a comment.Nov 20 2017, 1:55 PM

Looks OK to me. I'll defer to gtbercea@ for the final stamp.

test/Driver/openmp-offload-gpu.c
34

Please split long RUN lines. here and below.

This revision is now accepted and ready to land.Nov 20 2017, 4:47 PM
This revision was automatically updated to reflect the committed changes.
Hahnfeld marked an inline comment as done.