Page MenuHomePhabricator

[OpenMPIRBuilder][Fix] Move llvm::omp::types to OpenMPIRBuilder.
ClosedPublic

Authored by sstefan1 on Jul 5 2020, 7:25 AM.

Details

Summary

D82193 exposed a problem with global type definitions in
OMPConstants.h. This causes a race when running in thinLTO mode.
Types now live inside of OpenMPIRBuilder to prevent this from happening.

Diff Detail

Event Timeline

sstefan1 created this revision.Jul 5 2020, 7:25 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 5 2020, 7:25 AM
sstefan1 marked an inline comment as done.Jul 5 2020, 7:34 AM
sstefan1 added a subscriber: hoyFB.

Since this is not a small change, I think it would be good if @hoyFB could test if this resolves the issue.

There is one small issue though:

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
312

I wasn't sure how to handle __VA_ARGS__ here, since we would need OMPBuilder in front of every type.
That is why helper macros above exist. The problem with this is that this creates some unused variables in OpenMPOpt.

Not sure if -Wno-unused-variable would be a good thing to do temporarily? Is there another way to handle __VA_ARGS__ here?

sstefan1 updated this revision to Diff 275549.Jul 5 2020, 7:40 AM

small fix

hoyFB added a comment.EditedJul 5 2020, 5:22 PM

Thanks for the quick turnaround! I confirm the change fixes our build break.

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
312

Could it be possible to use OMPBuilder. _Name, OMPBuilder. _ReturnType everywhere?

jdoerfert accepted this revision.Jul 7 2020, 5:08 PM

LGTM.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1125

While we are here, remove uninitializeTypes

llvm/lib/Transforms/IPO/OpenMPOpt.cpp
312

Since this will (soonish) be replaced by tablegen definitions we can apply the following workaround:

Keep your helper macros but add
(void) VarName;
after each declaration :)

This revision is now accepted and ready to land.Jul 7 2020, 5:08 PM
This revision was automatically updated to reflect the committed changes.