This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Unify linking of OpenMP runtime. NFCI.
ClosedPublic

Authored by Hahnfeld on Feb 17 2017, 4:05 AM.

Diff Detail

Event Timeline

Hahnfeld created this revision.Feb 17 2017, 4:05 AM
pirama added inline comments.Feb 17 2017, 10:18 AM
lib/Driver/Tools.cpp
8683

addOpenMPRuntime would add -lomptarget even for this call if JA satisfies the newly added check. That's a deviation from exisiting behavior. Am I missing some invariant here where JobActions along this path don't have an offloading action?

Hahnfeld added inline comments.Feb 17 2017, 10:40 AM
lib/Driver/Tools.cpp
8683

Right, as I noted in the summary: This enables libomptarget for Darwin, FreeBSD and NetBSD

I don't see a problem with that, it's rather an inconsistency in the current code.

hfinkel added inline comments.Mar 1 2017, 3:50 PM
lib/Driver/Tools.cpp
10334–10337

Now that you've moved the comment, it is not clear what "this" means here. Please reword this to say that you should only pass true for GompNeedsRT on platforms that really need it.

Hahnfeld updated this revision to Diff 90295.Mar 2 2017, 1:01 AM
Hahnfeld marked an inline comment as done.

Reword comment

Hahnfeld updated this revision to Diff 90987.Mar 8 2017, 1:49 AM

Rebase for recent refactoring and ping.

Another ping

Hahnfeld updated this revision to Diff 93213.Mar 27 2017, 11:38 PM

Rebase and ping.

ABataev added inline comments.Apr 5 2017, 12:44 PM
lib/Driver/ToolChains/CommonArgs.cpp
430 ↗(On Diff #93213)

Do you really need to pass a reference to JobAction here or it is enough to pass a bool value for JA.isHostOffloading()?

443–445 ↗(On Diff #93213)

Remove braces here

Hahnfeld updated this revision to Diff 94372.Apr 6 2017, 7:49 AM
Hahnfeld marked 2 inline comments as done.
Hahnfeld retitled this revision from [Driver] Unify linking of OpenMP runtime to [Driver] Unify linking of OpenMP runtime. NFCI..
Hahnfeld edited the summary of this revision. (Show Details)
Hahnfeld added inline comments.
lib/Driver/ToolChains/CommonArgs.cpp
430 ↗(On Diff #93213)

Good idea, this even allows this change to become fully NFC

emaste added a subscriber: dim.Apr 6 2017, 10:11 AM
This revision is now accepted and ready to land.Apr 19 2017, 6:58 AM
This revision was automatically updated to reflect the committed changes.