This is an archive of the discontinued LLVM Phabricator instance.

Generate the capture for field when the field is used in openmp region with implicit default in the member function.
ClosedPublic

Authored by jyu2 on Jun 14 2022, 3:06 PM.

Details

Summary

This is to fix assert when field is referenced in openmp right with default (first|private) in member function.

The problem of assert is the capture is not generated for the field.

This patch is to generate capture when the field is used with implicit default, use it in the code, and save the capture off to make sure it is considered from that point and add first/private clauses.

1> Add new field ImplicitDefaultFirstprivateFDs in SharingMapTy, to store

generated capture fields info.

2> In function isOpenMPCaptureDecl: the caputer is generated and saved

in ImplicitDefaultFirstprivateFDs.

3> Add new help functions:

getImplicitFDCapExprDecl
isImplicitDefaultFirstprivateFD
addImplicitDefaultFirstprivateFD

4> Add addition argument in hasDSA to check default attribute for

default(first|private).

5> The isImplicitDefaultFirstprivateFD is used in VisitDeclRefExpr and

when building the implicit clause.

6> Add new parameter "Context" for buildCaptureDecl, due to when capture

field, the parent context is needed to be used.

7> Change isOpenMPPrivateDecl where stop propagate the capture from the

enclosing region for private variable.

8> In ActOnOpenMPFirstprivate/ActOnOpenMPPrivate, using captured info

to generate first|private clause.

Diff Detail

Event Timeline

jyu2 created this revision.Jun 14 2022, 3:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 3:06 PM
jyu2 requested review of this revision.Jun 14 2022, 3:06 PM

Why do we need to insert new implicit DSA? Only explicit DSAs are expected to be stored, implicit ones can be deduced using the rules.

jyu2 updated this revision to Diff 438877.Jun 21 2022, 5:29 PM
jyu2 edited the summary of this revision. (Show Details)

Thanks Alexey's comment. Address his comment. Instead save info into DSA, add new field in ImplicitDefaultFirstprivateFDs in SharingMapTy.

ABataev added inline comments.Jun 22 2022, 6:53 AM
clang/lib/Sema/SemaExprMember.cpp
1873–1877 ↗(On Diff #438877)

Why do we need this function? Implicit private rule should apply (if should) only to this-Юашдув kind of expressions, if something like parent.field expression is used, parent should be private, not private.field. Or I'm missing something?

clang/lib/Sema/SemaOpenMP.cpp
201

///

1141–1142
  1. Use /// style of comment.
  2. const member function?
1153

const auto &?

1158–1159
  1. Use /// style of comment.
  2. const member function?
1167

const auto &?

1172

/// style

1179–1180

just emplace_back(FD, Sz, VD);?

2390

Better to add an actual param type here but without Param name

jyu2 updated this revision to Diff 439195.Jun 22 2022, 4:00 PM
jyu2 marked 7 inline comments as done.
jyu2 retitled this revision from Generate the capture for field when the field is used with implicit default. to Generate the capture for field when the field is used in openmp region with implicit default in the member function..
jyu2 edited the summary of this revision. (Show Details)

Thanks Alexey for the review. This is address Alexey's comments.

jyu2 added inline comments.Jun 22 2022, 4:01 PM
clang/lib/Sema/SemaExprMember.cpp
1873–1877 ↗(On Diff #438877)

You are right for "parent.a" only privatize "parent".

But if 'a' is a field access in a member function then 'a' is privatized, not "this". It is same with firstprivate(a). But for the explicit firstprivate(a), the capture is build in ActOnOpenMPFirstprivateClause, so it can be mapped to reference in the omp region. For Implicit, I need to build capture for the first reference in the omp region with defalut(first|private) is used. And used that to generate firstprivate clause at end when the call to ActOnOpenMPFirstprivateClause when generating implicit clause.

Is this reasonable?

clang/lib/Sema/SemaOpenMP.cpp
201

Thanks Changed

2390

Changed.

ABataev added inline comments.Jun 23 2022, 4:08 AM
clang/lib/Sema/SemaExprMember.cpp
1873–1877 ↗(On Diff #438877)

But you don't need all this info to build the capture. I think you can reuse existing isOpenMPCapturedDecl function without adding this extra isOpenMPFDCaptureDecl. Most probably, you don't need Base and all other stuff to build this->fd member expression.

jyu2 updated this revision to Diff 439571.Jun 23 2022, 5:16 PM
jyu2 edited the summary of this revision. (Show Details)

Thanks Alexey's review. Address Alexey's review.

clang/lib/Sema/SemaExprMember.cpp
1873–1877 ↗(On Diff #438877)

Okay, I moved my code into isOpenMPCapturedDecl.

ABataev added inline comments.Jun 24 2022, 6:35 AM
clang/lib/Sema/SemaOpenMP.cpp
2421

Maybe BuildCXXThisExpr(SourceLocation(), getCurrentThisType(), /*IsImplicit=*/true)?

2433

Just DVarPrivate.CKind != OMPC_private

clang/lib/Sema/TreeTransform.h
11120–11121

Why do we need this check here?

jyu2 updated this revision to Diff 439760.Jun 24 2022, 7:57 AM
jyu2 marked an inline comment as done.

Address Alexey's comments! Thanks.

jyu2 added inline comments.Jun 24 2022, 8:00 AM
clang/lib/Sema/SemaOpenMP.cpp
2421

Thanks. Change

2433

Yes!! changed

clang/lib/Sema/TreeTransform.h
11120–11121

Without, the field is not getting rebuild during the template instantiation. That cause the field is not getting captured and implicit firstprivate clause is not getting generate. The test is added in default_firstprivate_ast_print.cpp
where I add check for it on line 58:
DUMP-NEXT: -OMPFirstprivateClause
DUMP-NEXT: -DeclRefExpr {{.*}} 'targetDev'

ABataev added inline comments.Jun 24 2022, 8:04 AM
clang/lib/Sema/TreeTransform.h
11120–11121

This should work without changes. Do we need clause in the template function?

jyu2 added inline comments.Jun 24 2022, 8:53 AM
clang/lib/Sema/TreeTransform.h
11120–11121

Do you mean, we don't need generate clause for instantiation's template function?

apply<32>
without this change the dump like:

| | `-CXXMethodDecl 0x1617ab8 <line:38:3, line:50:3> line:38:8 imported used apply 'void ()'
| |   |-TemplateArgument integral 32
| |   `-CompoundStmt 0x161c3a0 <col:16, line:50:3>
| |     `-OMPParallelDirective 0x161c350 <line:39:1, col:43>
| |       |-OMPDefaultClause 0x161bf30 <col:22, col:42>
| |       `-CapturedStmt 0x161c310 <line:40:5, line:42:5>

With this change
| | `-CXXMethodDecl 0x110cab8 <line:38:3, line:50:3> line:38:8 imported used apply 'void ()'
| |   |-TemplateArgument integral 32
| |   `-CompoundStmt 0x1111860 <col:16, line:50:3>
| |     `-OMPParallelDirective 0x1111808 <line:39:1, col:43>
| |       |-OMPDefaultClause 0x1110f30 <col:22, col:42>
| |       |-OMPFirstprivateClause 0x11117c8 <<invalid sloc>> <implicit>
| |       | `-DeclRefExpr 0x1111790 <line:41:7> 'int' lvalue OMPCapturedExpr 0x1111320 'targetDev' 'int &'
| |       `-CapturedStmt 0x1111590 <line:40:5, line:42:5>
ABataev added inline comments.Jun 24 2022, 9:35 AM
clang/lib/Sema/TreeTransform.h
11120–11121

It should work without this change correctly. If not, need to adjust sema analysis for such templated functions/fields.

jyu2 updated this revision to Diff 439812.Jun 24 2022, 10:11 AM

Remove change in TreeTransform.h. Will deal that in other patch. Thanks.

clang/lib/Sema/TreeTransform.h
11120–11121

Okay, I remove that change. Will deal that later.
It seems I need to set AlwaysRebuild to true. But I don't know how that works at this moment. Will submit other patch for this.

Thanks.

ABataev added inline comments.Jun 24 2022, 10:15 AM
clang/lib/Sema/TreeTransform.h
11120–11121

No, need to tweak the function that builds implicit clauses to build such clauses for non-templated decls, even in the template context

ABataev added inline comments.Jun 24 2022, 10:23 AM
clang/lib/Sema/SemaOpenMP.cpp
201

What is Sz here? Better to give a better name and add a description for the struct and all fields

1149

Is it possible to have an overflow here?

1154

What if Sz == -1?

1182

What about overflow here?

17803–17813

A check here not for curcontext dependent but for FD being dependent?

18082

Adjust this check for dependent context with non-dependent FD?

jyu2 updated this revision to Diff 439906.Jun 24 2022, 3:27 PM
jyu2 added inline comments.Jun 24 2022, 3:33 PM
clang/lib/Sema/SemaOpenMP.cpp
201

How about use StackLevel. Sorry use 0 instead

1149

I don't think that is possible. when I != EndI and Stacklevel should > 0 Or if I == EndI and Stacklevel should be 0

I add assert to check boundary condition be ensure:
assert((StackLevel > 0 && I != EndI) || (StackLevel == 0 && I == EndI));

1154

Should not be 0.

1182

Add assert for boundary check.

17803–17813

I can not this to work. Since the private copy only build under only build non-dependent context.

jyu2 updated this revision to Diff 441039.Jun 29 2022, 9:11 AM

Thanks Alexey for the review.
Add change in TreeTransform.h to rebuilt member expression.

ABataev added inline comments.Jun 29 2022, 11:59 AM
clang/lib/Sema/SemaOpenMP.cpp
199

Add comments for the structure and all fields, please.

2421

Do we need to check if getCurrentThisType() returns nullptr?

clang/lib/Sema/TreeTransform.h
11125

Can we also check if the field should be made private, i.e. avoid this short circuit if it is not in OpenMP region or is not supposed to be perivitized in the the context (no default private/firstprivate in the region)?

jyu2 updated this revision to Diff 441257.Jun 29 2022, 10:01 PM

Thanks Alexey for the review.
This is to address Alexey's comments,

ABataev added inline comments.Jun 30 2022, 4:10 AM
clang/lib/Sema/SemaOpenMP.cpp
199

Data required?

200

default

212

Captured?

clang/lib/Sema/TreeTransform.h
122

I think we don't need to add a new field here. Can instead we have a check for regions with default clauses, if possible?

jyu2 added inline comments.Jun 30 2022, 9:04 AM
clang/lib/Sema/SemaOpenMP.cpp
17803–17813

fix typo:
I can not get this to work, since the private copy only build under non-dependent context.

clang/lib/Sema/TreeTransform.h
122

It seems during the TransformMemberExpr, I can not get Directives info for omp regions to check. Could you give me hand?
Thanks.

ABataev added inline comments.Jun 30 2022, 12:47 PM
clang/lib/Sema/TreeTransform.h
122

Yes, we do not rebuild the DSA stack at the time of the instantiation. Can you just check that we're inside OpenMPCapturedRegion? Something like getCurCapturedRegion()->CapRegionKind == CR_OpenMP? Or walk the stack of regions to find the outer OpenMP region, if any.

jyu2 updated this revision to Diff 441565.Jun 30 2022, 6:14 PM

Thanks Alexey for the review. This patch to address his comment.

jyu2 added inline comments.Jun 30 2022, 6:16 PM
clang/lib/Sema/SemaOpenMP.cpp
212

Fixed. Thanks.

clang/lib/Sema/TreeTransform.h
122

Thank you so much for the help. I add new function in Sema to do this. Hope that is okay with you.

ABataev added inline comments.Jul 1 2022, 8:36 AM
clang/lib/Sema/SemaOpenMP.cpp
2273–2274

What if we have another outer OpenMP region, something like lambda inside OpenMP region?

jyu2 added inline comments.Jul 1 2022, 9:20 AM
clang/lib/Sema/SemaOpenMP.cpp
2273–2274

My understanding is that hasDSA will go up to find innermost openmp region which has default clause. Am I right here?

ABataev added inline comments.Jul 1 2022, 9:23 AM
clang/lib/Sema/SemaOpenMP.cpp
2273–2274

Yes, if you're immediate captured region is OpenMP region. But what if you're inside lambda, which is inside OpenMP region? In this case getCurCapturedRegion()->CapRegionKind != CR_OpenMP. Will it still work, could add a test for this situation?

jyu2 added inline comments.Jul 1 2022, 12:02 PM
clang/lib/Sema/SemaOpenMP.cpp
2273–2274

Good catch Thank you so much You are right. I removed check for CR_OpenMP. Just let hasDSA to find out. And add test for this.

jyu2 updated this revision to Diff 441765.Jul 1 2022, 12:04 PM

Thank Alexey for the review. This is remove check for omp region just use hasDSA.

This revision is now accepted and ready to land.Jul 1 2022, 12:16 PM
This revision was landed with ongoing or failed builds.Jul 1 2022, 5:33 PM
This revision was automatically updated to reflect the committed changes.
mgabka added a subscriber: mgabka.Aug 2 2022, 4:27 AM

I noticed that this patch is causing now an assertion failure for cases like :

class A{

void a() {
  #pragma omp parallel
  a(b);
}

};

The failed assertion is: "const clang::ValueDecl* getCanonicalDecl(const clang::ValueDecl*): Assertion `FD' failed."

while before it clang was correctly reporting error:
error: use of undeclared identifier 'b'

is it the same assertion you were trying to fix here?

jyu2 added a comment.Aug 2 2022, 8:06 AM

I noticed that this patch is causing now an assertion failure for cases like :

class A{

void a() {
  #pragma omp parallel
  a(b);
}

};

The failed assertion is: "const clang::ValueDecl* getCanonicalDecl(const clang::ValueDecl*): Assertion `FD' failed."

while before it clang was correctly reporting error:
error: use of undeclared identifier 'b'

is it the same assertion you were trying to fix here?

No, the assert I am fixing is when default(firstprivate) is used inside member function.

I noticed that this patch is causing now an assertion failure for cases like :

class A{

void a() {
  #pragma omp parallel
  a(b);
}

};

The failed assertion is: "const clang::ValueDecl* getCanonicalDecl(const clang::ValueDecl*): Assertion `FD' failed."

while before it clang was correctly reporting error:
error: use of undeclared identifier 'b'

is it the same assertion you were trying to fix here?

No, the assert I am fixing is when default(firstprivate) is used inside member function.

Could you please investigate and fix it?

jyu2 added a comment.Aug 2 2022, 8:31 AM

I noticed that this patch is causing now an assertion failure for cases like :

class A{

void a() {
  #pragma omp parallel
  a(b);
}

};

The failed assertion is: "const clang::ValueDecl* getCanonicalDecl(const clang::ValueDecl*): Assertion `FD' failed."

while before it clang was correctly reporting error:
error: use of undeclared identifier 'b'

is it the same assertion you were trying to fix here?

No, the assert I am fixing is when default(firstprivate) is used inside member function.

Could you please investigate and fix it?

Yes, I will fix this.

mgabka added a comment.Aug 2 2022, 9:19 AM

I noticed that this patch is causing now an assertion failure for cases like :

class A{

void a() {
  #pragma omp parallel
  a(b);
}

};

The failed assertion is: "const clang::ValueDecl* getCanonicalDecl(const clang::ValueDecl*): Assertion `FD' failed."

while before it clang was correctly reporting error:
error: use of undeclared identifier 'b'

is it the same assertion you were trying to fix here?

No, the assert I am fixing is when default(firstprivate) is used inside member function.

Could you please investigate and fix it?

Yes, I will fix this.

Thanks a lot, I raised an upstream bug for it https://github.com/llvm/llvm-project/issues/56884