Page MenuHomePhabricator

[OpenMP] Expand short verisions of OpenMP offloading triples
ClosedPublic

Authored by jhuber6 on Jan 18 2022, 7:27 PM.

Details

Summary

The OpenMP offloading libraries are built with fixed triples and linked
in during compile time. This would cause un-helpful errors if the user
passed in the wrong expansion of the triple used for the bitcode
library. because we only support these triples for OpenMP offloading we
can normalize them to the full verion used in the bitcode library.

Event Timeline

jhuber6 created this revision.Jan 18 2022, 7:27 PM
jhuber6 requested review of this revision.Jan 18 2022, 7:27 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 18 2022, 7:27 PM

Can we have a test? Similar to a/the test introduced with the libdevice handling.

jhuber6 updated this revision to Diff 401252.Jan 19 2022, 8:19 AM

Adding tests.

This revision is now accepted and ready to land.Jan 19 2022, 8:26 AM
JonChesterfield requested changes to this revision.EditedJan 19 2022, 9:47 AM

Wrong fix. The user typing -fopenmp-targets=amdgcn is an error, we shouldn't swallow errors here and hope the backend does something sane with the result. We should rewrite that in clang to the right triple.

This may 'work', if the correct triple is the one which wins the conflict, provided nothing triple-dependent has already happened in the IR, and we never target another OS, but that's much less robust than setting the HSA triple directly.

This revision now requires changes to proceed.Jan 19 2022, 9:47 AM
arsenm added a subscriber: arsenm.Jan 19 2022, 9:54 AM
arsenm added inline comments.
llvm/lib/Linker/IRMover.cpp
1482 ↗(On Diff #401252)

I'm a bit disturbed the linker is special casing bitcode file names

jhuber6 retitled this revision from [OpenMP] Add triple warning exceptions for OpenMP bitcode library to [OpenMP] Expand short verisions of OpenMP offloading triples.Jan 19 2022, 11:54 AM
jhuber6 edited the summary of this revision. (Show Details)
jhuber6 updated this revision to Diff 401357.Jan 19 2022, 11:54 AM
jhuber6 edited the summary of this revision. (Show Details)

Changing approach to simply expand the triple where we parse it for OpenMP.

Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 11:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
JonChesterfield accepted this revision.Jan 19 2022, 12:26 PM

LG, thanks!

clang/lib/Driver/Driver.cpp
778 ↗(On Diff #401357)

Might be worth mentioning in the comment that this is essentially a user convenience feature. We're not so much normalising, as treating fopenmp-target=nvtpx64 as an abbreviation for nvptx64-nvidia-cuda

This revision is now accepted and ready to land.Jan 19 2022, 12:26 PM