Page MenuHomePhabricator

[OpenMP 5.0] - Extend defaultmap
ClosedPublic

Authored by cchen on Oct 18 2019, 3:23 PM.

Details

Summary

For the extended defaultmap, most of the work is inside sema.
The only difference for codegen is to set different initial
maptype for different implicit-behavior.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ABataev added inline comments.Mon, Nov 11, 7:56 AM
clang/lib/Sema/SemaOpenMP.cpp
2838–2857

Why inner switch for the same variable DMIB?

4435

Use preincrement instead of postincrement.

4475

Use range-based loop here.

4476

Simplify inner complexity here by using continue; if the condition is true.

16412–16435

Do we need types DefaultMapImplicitBehavior and DefaultMapVariableCategory if the map 1 to 1 to OpenMPDefaultmapClauseModifier and OpenMPDefaultmapClauseKind?

cchen marked an inline comment as done.Mon, Nov 11, 9:12 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
4475

But in line 4480, I require the index of the iteration. If I use range based for loop, then I'll need to get the index like addressof(ImplicitMap) - addressof(ImplicitMaps[0]), which I'm not sure I should write code like this.

ABataev added inline comments.Mon, Nov 11, 9:21 AM
clang/lib/Sema/SemaOpenMP.cpp
4475

Better to use a new counter var here for ClauseKind

cchen updated this revision to Diff 228761.Mon, Nov 11, 12:52 PM

Fix based on feedback

ABataev added inline comments.Mon, Nov 11, 1:29 PM
clang/lib/Sema/SemaOpenMP.cpp
1896–1898

Just DSAStack->isDefaultmapCapturedByRef(Level, getVariableCategoryFromDecl(D))

2114–2116

Just DSAStack->mustBeFirstprivateAtLevel(NewLevel, getVariableCategoryFromDecl(D))

16412–16435

What about this?

cchen marked an inline comment as done.Mon, Nov 11, 1:33 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
16412–16435

The value of OpenMPDefaultmapClauseModifier does not start from one (due to the design in parsing I guess) so I'm not able to do so.

cchen updated this revision to Diff 228768.Mon, Nov 11, 1:51 PM

Refactor by taking advantage of helper

ABataev added inline comments.Tue, Nov 12, 8:57 AM
clang/lib/Parse/ParseOpenMP.cpp
2313–2320

I believe this problem exists in the original code? If so, better to split this patch and commit this fix in the separate patch.

clang/lib/Sema/SemaOpenMP.cpp
2999–3001

Stack->mustBeFirstprivate(DMVC)?

4438

ImplicitMaps[I].append(ImplicitMap.begin(), ImplicitMap.end());

16412–16435

No, I meant that the enumerics are almost the same. Can we reuse the original one rather than adding 2 new enums with almost the same members and the same meanings?

cchen marked 2 inline comments as done.Tue, Nov 12, 10:12 AM
cchen added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
2313–2320

Yes, this problem is from the original code, but bool isDefaultmapModifier = (M != OMPC_DEFAULTMAP_MODIFIER_unknown); at line 16437 relies on this change. Otherwise, I'll have to write (M == OMPC_DEFAULTMAP_MODIFIER_to) || (M == OMPC_DEFAULTMAP_MODIFIER_from) ||....
So do you think that I should just write (M == OMPC_DEFAULTMAP_MODIFIER_to) || (M == OMPC_DEFAULTMAP_MODIFIER_from) ||... or M > OMPC_DEFAULTMAP_MODIFIER_unknown for this patch and fix them in another patch?

clang/lib/Sema/SemaOpenMP.cpp
16412–16435

Got it, I'll use OpenMPDefaultmapClauseModifier and OpenMPDefaultmapClauseKind instead. Thanks

ABataev added inline comments.Tue, Nov 12, 10:27 AM
clang/lib/Parse/ParseOpenMP.cpp
2313–2320

You will just commit the second patch at first and then just rebase this patch.

cchen updated this revision to Diff 229106.Wed, Nov 13, 8:09 AM

Remove unnecessary enums and do some renaming

ABataev added inline comments.Wed, Nov 13, 8:33 AM
clang/lib/Sema/SemaOpenMP.cpp
641

Static function

3089

true->/*Cond=*/true

4460

for (ArrayRef<Expr *> ImplicitMaps: ImplicitMaps)

16466

Not formatted

cchen updated this revision to Diff 229115.Wed, Nov 13, 8:55 AM

Fix based on feedback

cchen updated this revision to Diff 229119.Wed, Nov 13, 9:09 AM
cchen marked 28 inline comments as done.

fix a misunderstood change

Also, one common comment. It would be good to reduce copy-paste in tests and just add some new lines of codes, not to repeat existing one.

clang/test/OpenMP/target_defaultmap_messages.cpp
1

What about the testing for the previous version of OpenMP, 45?

cchen marked 25 inline comments as done.Wed, Nov 13, 9:13 AM
cchen updated this revision to Diff 229169.Wed, Nov 13, 1:08 PM

Remove duplicate code in test

cchen marked an inline comment as done.Wed, Nov 13, 1:11 PM
ABataev added inline comments.Wed, Nov 13, 1:15 PM
clang/test/OpenMP/target_defaultmap_messages.cpp
49

Same test case must exist for OpenMP 4.5

cchen marked an inline comment as done.Wed, Nov 13, 1:26 PM
cchen added inline comments.
clang/test/OpenMP/target_defaultmap_messages.cpp
49

This test is only for testing the new rule in OpenMP 5.0 which disallow duplicate variable-category in defaultmap. For OpenMP 4.5, only one defaultmap is allowed, and I think I didn't remove the test for that by this one.

ABataev added inline comments.Wed, Nov 13, 1:30 PM
clang/test/OpenMP/target_defaultmap_messages.cpp
49

We don't have such test for OpenMP 4.5 and it would be good to have it

cchen marked an inline comment as done.Wed, Nov 13, 1:33 PM
cchen added inline comments.
clang/test/OpenMP/target_defaultmap_messages.cpp
49

I'm glad to add the test (duplicate defaultmap) for OpenMP 4.5 defaultmap. But just to clarify, are you also saying that I should remove the ifdef OMP5 and add the error message for OpenMP 4.5?

ABataev added inline comments.Wed, Nov 13, 1:35 PM
clang/test/OpenMP/target_defaultmap_messages.cpp
49

Yes, you can move ifdef lower and test it for both modes, 50 and 45. Also note, that the original message from OpenMP 4.5 must be emitted in 4.5 mode, not the new one from OpenMP 5.0

cchen updated this revision to Diff 229185.Wed, Nov 13, 2:10 PM

Add a test for OpenMP4.5 defaultmap and add the message back for Clang

ABataev added inline comments.Thu, Nov 14, 8:05 AM
clang/lib/Parse/ParseOpenMP.cpp
2023–2029

I think better just to modify the condition in the original if:

if ((getLangOpts().OpenMP < 50 || CKind != OMPC_defaultmap) && !FirstClause)
clang/lib/Sema/SemaOpenMP.cpp
118

Kind is unused here, do you really need it?

2822

Better to rename Cond to something like IsAggregateOrDeclareTarget

2846–2850
if (Cond) {
  Kind = OMPC_MAP_tofrom;
  break;
}
llvm_unreachable("Unexpected defaultmap implicit behavior");
2953–2954

Why comparing the value of *Res here if just comparing against all possible values? This condition is always true if Res is true. I don't think we need to map variables with to mapping.

cchen updated this revision to Diff 229326.Thu, Nov 14, 9:08 AM

Fix based on feedback

ABataev added inline comments.Thu, Nov 14, 9:47 AM
clang/lib/Sema/SemaOpenMP.cpp
2846

Is this still true for decalare target to vars?

2953–2954

Do we need to map declare target to vars at all?

cchen marked 2 inline comments as done.Thu, Nov 14, 10:26 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
2953–2954

I don't understand this since spec says that

each variable referenced in the construct that does not have a predetermined data-sharing attribute and does not appear in a to or link clause on a declare target directive must be listed in a data-mapping attribute clause, a data-sharing attribute clause (including a data-sharing attribute clause on a combined construct where target is one of the constituent constructs), or an is_device_ptr clause.

So, I think I should check both to and link.
Or are you saying that to clause is impossible to happen here since we have a "Skip internally declared static variables" check so that if there is a to clause, it will return before hitting Line 2952. Therefore, we should only check for link clause?

ABataev added inline comments.Thu, Nov 14, 11:10 AM
clang/lib/Sema/SemaOpenMP.cpp
2952

Why extra parens in the condition?

2953–2954

Missed ! in the condition. Why we check for !isOpenMPTargetExecutionDirective(DKind)

cchen updated this revision to Diff 229374.Thu, Nov 14, 11:51 AM

Remove redundant code

Tests for codegen of the declare target to|link vars with the new defaultmap clauses?

cchen updated this revision to Diff 229392.Thu, Nov 14, 1:23 PM

Add test about declare target link/to

ABataev added inline comments.Thu, Nov 14, 1:33 PM
clang/test/OpenMP/target_defaultmap_codegen.cpp
1539–1547

What does this test check? It is not the test for declare target link. Also, A is calar and defaultmap is for aggregates.

cchen marked an inline comment as done.Thu, Nov 14, 1:55 PM
cchen added inline comments.
clang/test/OpenMP/target_defaultmap_codegen.cpp
1539–1547

My bad, I forgot to update from target to(Vector) to target link(Vector). And my intention for the aggregate is for Vector[1024] not for A. The confusion is due to my negligence, I'll fix it right away. Thanks for your passion.

cchen updated this revision to Diff 229399.Thu, Nov 14, 1:57 PM

Fix a silly bug in test

ABataev added inline comments.Fri, Nov 15, 7:09 AM
clang/test/OpenMP/target_defaultmap_codegen.cpp
1545–1546

Don't understand how does it test mapping of the declare target link vars.

cchen updated this revision to Diff 229572.Fri, Nov 15, 9:24 AM

Fix test

ABataev accepted this revision.Fri, Nov 15, 9:46 AM

LG with a nit

clang/lib/Sema/SemaOpenMP.cpp
4459–4463

unsigned->int

This revision is now accepted and ready to land.Fri, Nov 15, 9:46 AM
cchen updated this revision to Diff 229580.Fri, Nov 15, 9:51 AM

Fix a wrong type

cchen added a comment.Fri, Nov 15, 9:55 AM

Alexey, thanks so much for your help, you really make me learn so much about Clang and coding in general. Can you land this patch for me since I'm still new to llvm/clang and haven't have the commit access. Thanks!

Alexey, thanks so much for your help, you really make me learn so much about Clang and coding in general. Can you land this patch for me since I'm still new to llvm/clang and haven't have the commit access. Thanks!

Sure, will do. Thanks for the patch!

This revision was automatically updated to reflect the committed changes.