This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Bugfix for processing of global variables in OpenMP regions.
ClosedPublic

Authored by ABataev on Nov 13 2014, 9:29 PM.

Details

Summary

Currently, if global variable is marked as a private OpenMP variable, the compiler crashes in debug version or generates incorrect code in release version. It happens because in the OpenMP region the original global variable is used instead of the generated private copy. It happens because currently globals variables are not captured in the OpenMP region.
This patch adds capturing of global variables iff private copy of the global variable must be used in the OpenMP region.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 16194.Nov 13 2014, 9:29 PM
ABataev retitled this revision from to [OPENMP] Bugfix for processing of global variables in OpenMP regions..
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).
rjmccall edited edge metadata.Nov 17 2014, 3:14 PM

Ugh. Copy-captured global variables? Really?

Well, it does seem to be very explicitly specified that way, and it's also very explicitly specified that references to the original global (e.g. from outside the pragma) may or may not refer to the private copy. So I guess this is what we're doing now.

I think it would make more sense to track the reference-to-capture / reference-to-non-capture difference reliably in the AST. We already have a refersToEnclosingLocal() bit on DeclRefExpr; making that a general "this is a reference to a captured variable" flag makes sense. We then won't have to recompute it during IRGen.

lib/Sema/SemaExpr.cpp
12119 ↗(On Diff #16194)

This comment needs to be changed, I guess.

12122 ↗(On Diff #16194)

Please avoid even making a call for globals if we aren't in OpenMP mode. (NeedToCapture will be reliably inlined; IsOpenMPPrivateVar won't be. You can make IsOpenMPPrivateVar assert that we're in OpenMP mode.)

Hi John,
Thanks a lot for the review.

Ugh. Copy-captured global variables? Really?

Yes, if you have global var explicitly marked as private in OpenMP region, like:

static int g;
void foo() {
int i;
#pragma omp parallel firstprivate(g)
i = ++g;
}

you have to capture 'g' and generate a private copy with initial value of 'g'.

I think it would make more sense to track the reference-to-capture / reference-to-non-capture difference reliably in the AST. We already have a refersToEnclosingLocal() bit on DeclRefExpr; making that a general "this is a reference to a captured variable" flag makes sense. We then won't have to recompute it during IRGen.

I'll try to implement it. I ran into several troubles in my first attempt, trying to resolve them now.

lib/Sema/SemaExpr.cpp
12119 ↗(On Diff #16194)

Ok, I'll change it.

12122 ↗(On Diff #16194)

Actually IsOpenMPPrivateVar() already checks that OpenMP mode is on. But I can add a check for OpenMP mode like 'LangOpts.OpenMP && IsOpenMPPrivateVar()'.

ABataev updated this revision to Diff 16473.Nov 20 2014, 10:46 PM
ABataev edited edge metadata.

Fixes after review. Renamed DeclRefExprBits.RefersToEnclosingLocal and corresponding references to DeclRefExprBits.RefersToCapturedVariable.

John, May I ask you to look at this patch once again?

Sorry, lost track of it. Thanks for pinging.

lib/CodeGen/CGExpr.cpp
1912 ↗(On Diff #16473)

This should be exhaustive, right? In what situation can we have a DRE that refersToCapturedVariable() and we're not in a lambda, captured statement, or block?

If so, you should just use cast<> here.

lib/Sema/SemaExpr.cpp
12141 ↗(On Diff #16473)

I think this is correct, but it deserves a comment. We need to check for the parent *first* because, if we *have* private-captured a global variable, we need to recursively capture it in intermediate blocks, lambdas, etc.

Also, instead of recomputing hasLocalStorage(), please remember it from above.

12373 ↗(On Diff #16473)

This method name suggests that it's non-state-changing, but I'm pretty sure that tryCaptureVariable isn't. Why do you need this routine at all?

lib/Sema/SemaOpenMP.cpp
555 ↗(On Diff #16473)

Just assert LangOpts.OpenMP.

ABataev updated this revision to Diff 17222.Dec 12 2014, 3:39 AM

Update after review

ABataev added inline comments.Dec 12 2014, 4:05 AM
lib/CodeGen/CGExpr.cpp
1912 ↗(On Diff #16473)

I had a lot of troubles with this, but I fixed it.

lib/Sema/SemaExpr.cpp
12141 ↗(On Diff #16473)

Ok, will do.

12373 ↗(On Diff #16473)

tryCaptureVariable is non-state-changing if BuildAndDiagnose arg is false. I need this function to check that we need to set flag RefersToCapturedVariable for DRE for the specified variable. (see line 1608 in lib/Sema/SemaExpr.cpp)

lib/Sema/SemaOpenMP.cpp
555 ↗(On Diff #16473)

Ok

This generally looks great, thanks. I'm thinking we should probably split the rename of refersToEnclosingLocal -> refersToCapturedVariable into a separate patch, though.

lib/CodeGen/CGExpr.cpp
1905 ↗(On Diff #17222)

It's possible to fall out of this entire block if CapturedStmtInfo->lookup(VD) fails. Is that expected to be possible? If not, you should have an assertion here. If it is, I don't know what you want to do, but there should at least be a comment explaining why this is possible.

ABataev updated this revision to Diff 17269.Dec 14 2014, 10:38 PM

Removed 'if (auto *FD = CapturedStmtInfo->lookup(VD))' from codegen for variable captured in CapturedStmt

I'm thinking we should probably split the rename of refersToEnclosingLocal -> refersToCapturedVariable into a separate patch, though.

I think we should keep it in this patch, because we're changing the meaning of this flag.

lib/CodeGen/CGExpr.cpp
1905 ↗(On Diff #17222)

John, I fixed it.

I'm thinking we should probably split the rename of refersToEnclosingLocal -> refersToCapturedVariable into a separate patch, though.

I think we should keep it in this patch, because we're changing the meaning of this flag.

We're expanding the meaning of the flag. It's good to stage changes like this — it means that you can say definitively that the larger and more invasive-looking patch is a non-functional change, which is really useful for people reviewing the commit history. Please do this; I know it's a little annoying to do it after you've already got a unified patch, but it's the right thing to do.

If you split it, it looks ready to go.

John, ok, I'll split it into two patches and commit them separately.

This revision was automatically updated to reflect the committed changes.