This is an archive of the discontinued LLVM Phabricator instance.

[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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cchen marked 12 inline comments as done.Oct 31 2019, 2:35 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
1906

I somehow forgot about this, will fix this soon.

ABataev added inline comments.Nov 1 2019, 7:24 AM
clang/include/clang/Basic/OpenMPKinds.def
438–448

Add some guards in the code, these values must be enabled only for OpenMP 5.0, for 4.5 only scalar:tofrom is allowed. Add a test that the error messages are emitted for new cases in OpenMP 4.5

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7274 ↗(On Diff #227350)

I think you just need to keep the original value of the Bits variable set in line 7242

7276 ↗(On Diff #227350)

It is not recommended to use default: case in switches over enumerics, just add a case for each enum value and remove default:

7368–7370 ↗(On Diff #227350)

Do you really need the default params values here?

clang/lib/Sema/SemaOpenMP.cpp
115

No need to call the constructor for classes as a default value, just SourceLocation SLoc; is enough

130

Maybe, it would be better to make DMVC_unspecified the last one in the DefaultMapVariableCategory and use it as an array dimension here rather than rely on the magical number?

603

Do you really need the static_cast here?

605

Do you really need this field if your DefaultmapMap already variable category based array?

610

Do you really need a cast here?

675

Better to use switch here.

676–692

Seems to me, these 2 checks are very similar, you cam merge them

2948

Just isAnyPointerType()

2952–2953

Just !VD->getType()->isAnyPointerType(). Plus, I think you won't need it here in case of fixed condition in the main if.

2956

Just isAggregateType()?

3004–3011

Also must include DMIB_firstprivate.

16419

The code is not formatted.

16478

Better to use switch here too

16501

No default

clang/test/OpenMP/target_defaultmap_codegen.cpp
45

Why it is allowed in OpenMP 4.5? I don't see an option in your tests for OpenMP 5.0. In 4.5 there must be an error.

cchen marked an inline comment as done.Nov 1 2019, 10:52 AM
cchen added inline comments.
clang/include/clang/Basic/OpenMPKinds.def
438–448

Do you mean that I should add the guards in this file (OpenMPKinds.def) so that Clang will not even parse those new keywords for OpenMP < 50 or I could just check the use of those keywords in Sema?
If I should put guards in the .def file, could you give me any hint or point me to an example? Thanks!

ABataev added inline comments.Nov 1 2019, 10:59 AM
clang/include/clang/Basic/OpenMPKinds.def
438–448

No, not in this file, this is just general comment. You need to guard this during semantic analysis

cchen updated this revision to Diff 227541.Nov 1 2019, 2:44 PM

Fix firstprivate issue and refactor based on feedback

cchen marked 20 inline comments as done.Nov 1 2019, 2:52 PM
cchen marked an inline comment as done.Nov 2 2019, 2:34 PM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7368–7370 ↗(On Diff #227350)

I'm doing so since ImplicitBehavior and VariableCategory is only used when the generateInfoForComponentList is called inside generateInfoForCapture, and for other control flow, such as generateInfoForComponentList called inside generateInfoForDeclareTargetLink, we don't need to set this two argument .

Some comments marked as ]Вщту] are not done actually, check them, please.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
7246–7248 ↗(On Diff #227541)

Hmm, this is strange, Do we really need this kind of processing here? The variables must be mapped implicitly in Sema and, thus, all this processing of the default mapping rules should not be required.

8342 ↗(On Diff #227541)

It should not be hardcoded, you need to teach sema about other possible values for map type.

clang/lib/Sema/SemaOpenMP.cpp
130

Not done.

cchen marked 2 inline comments as done.Nov 4 2019, 9:34 PM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7246–7248 ↗(On Diff #227541)

I'm now having design question about setting the correct implicit map type in Sema for the below situation:

int *ptr_1, *ptr_2, arr[50];
#pragma omp target defaultmap(alloc:pointer) defaultmap(from:aggregate)
{
  ptr_1++, ptr_2++;
  arr[0]++;
}

In this case we need to store two maptypes - alloc and from for an ActOnOpenMPMapClause but ActOnOpenMPMapClause only pass one maptype so I'm wondering should I modify the interface of OMPMapClause which pass an array of maptypes rather than one maptype variable?

clang/lib/Sema/SemaOpenMP.cpp
130

Not sure about this one. I've already put DMVC_unspecified to the last one in the DefaultMapVariableCategory enum so that I now don't need magic number. Or you are pointing something else? Thanks

ABataev added inline comments.Nov 5 2019, 6:58 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7246–7248 ↗(On Diff #227541)

Just create 3 arrays instead of single array for mapped items and call ActOnOpenMPMapClause for each of them

clang/lib/Sema/SemaOpenMP.cpp
130

Use DefaultmapInfo DefaultmapMap[DMVC_unspecified] instead of DefaultmapInfo DefaultmapMap[3], this is what I meant.

cchen updated this revision to Diff 228001.Nov 5 2019, 9:27 PM

Use sema to generate correct maptype instead of codegen.

ABataev added inline comments.Nov 6 2019, 6:59 AM
clang/lib/Sema/SemaOpenMP.cpp
1895

Not done

2825

No need else if here , just if

2827

No need for else here, just return

3223–3226

Better to get each particular list via the DMVC_ key passed as parameter

4406

OMPC_MAP_tofrom must be returned only for DMIB_tofrom, for others, it must be llvm_unreachable(....)

16549

Just create 2 variables and initialize them. Call setDefault... only once.

cchen marked 2 inline comments as done.Nov 6 2019, 8:39 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
1895

I think I check for pointer in line 1897-1898 or I could just misunderstand what you are saying.

4406

For DMIB_firstprivate, I think you are right, it should be unreachable, however, for DMIB_default and DMIB_unspecified, they are definitely reachable in ActOnOpenMPExecutableDirective.

ABataev added inline comments.Nov 6 2019, 8:49 AM
clang/lib/Sema/SemaOpenMP.cpp
4406

Then this function is not correct because for scalars default is not tofrom.

cchen marked an inline comment as done.Nov 6 2019, 9:38 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
4406

You are right, for scalar and pointer, the implicit behavior is firstprivate, so they are unreachable in this case, however, for aggregate, the implicit behavior is tofrom (I emit the .ll file for aggregate using the master branch and found that the maptype is 547).

ABataev added inline comments.Nov 6 2019, 9:43 AM
clang/lib/Sema/SemaOpenMP.cpp
4406

You need to add extra checks for scalars and pointers in this function.

cchen marked an inline comment as done.Nov 6 2019, 1:12 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
4406

In fact, it is possible for scalar or pointer to reach DMIB_default and DMIB_unspecified case. For this example:

extern int c;
#pragma omp declare target link(c)

void foo() {
  #pragma omp target defaultmap(default:scalar)
  {
    c++;
  }
}

c will be added into ImplicitMap since Res is not empty, therefore, DMIB_default and DMIB_unspecified is reachable.

ABataev added inline comments.Nov 6 2019, 1:18 PM
clang/lib/Sema/SemaOpenMP.cpp
4406

Again, it means that this function is not correct in all cases and should be reworked somehow or deleted.

cchen updated this revision to Diff 228142.Nov 6 2019, 2:10 PM

Remove incorrect DMIB to OMPC_MAP function and do some refactoring.

ABataev added inline comments.Nov 7 2019, 6:57 AM
clang/lib/Sema/SemaOpenMP.cpp
601

auto -> to real type

607

No need for explicit typecast, enums are implcitly casted to ints.

2120–2121

Why aggregates are firstprivate by default? I believe only scalars are firstprivates by default.

3223–3225

Revert back to ArrayRef

4485–4487

Use switch

16415
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16438
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16471–16480

Just M != OMPC_DEFAULTMAP_MODIFIER_unknown and Kind != OMPC_DEFAULTMAP_unknown

16486

Why need to add a new Loc var here? Use the required KindLoc\MLoc directly.

cchen marked an inline comment as done.Nov 7 2019, 9:38 AM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
16471–16480

Thanks for your kindly suggestion, but I'm not able to use M != OMPC_DEFAULTMAP_MODIFIER_unknown to deal with the case that the defaultmap modifier never being set. For this case:

#pragma omp target defaultmap(scalar:

The defaultmap modifier returned from the parsing phase is 0 while OMPC_DEFAULTMAP_MODIFIER_unknown is set to 3 (OMPC_DEFAULTMAP_unknown). I could make the condition correct by just comparing M with 0 but this is the use of magic number, so I'm wondering should I just comparing each case explicitly (alloc, to, from...) or I'll need to fix the parsing?

ABataev added inline comments.Nov 7 2019, 9:51 AM
clang/lib/Sema/SemaOpenMP.cpp
16471–16480

Could you set OMPC_DEFAULTMAP_MODIFIER_unknown as an initial value? Or set OMPC_DEFAULTMAP_MODIFIER_unknown as thr first enum with value 0 and instead add something like OMPC_DEFAULTMAP_MODIFIER_last and use it everywhere in the code where you need the number of values instead of OMPC_DEFAULTMAP_MODIFIER_unknown.

cchen updated this revision to Diff 228317.Nov 7 2019, 2:48 PM

Set initial defaultmap modifier to be unknown if not valid and fix based on feedback

cchen marked an inline comment as done.Nov 7 2019, 3:11 PM
cchen added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
4485–4487

In this switch statement, I checked if the implicit map contains any "declare target link" to prove that for the normal case (no declare target link/to), DMIB_default and DMIB_default is unreachable for scalar and pointer.
However, this change is not quite right since I haven't added any extra ImplicitMap to deal with the case that ImplicitMap contains "declare target link" variable. (instead, I only create an extra field in ImplicitMap only so that I can demonstrate) And the reason why I'm hesitant to do so is that adding another two ImplicitMap only for "declare target link" might be a little be overkill?

ABataev added inline comments.Nov 8 2019, 7:18 AM
clang/lib/Sema/SemaOpenMP.cpp
4485–4487

Then, I think, you just use the wrong key for the implicit mapping. You're using the kind of the mapped data (scalar, pointer or aggregate), instead used kind of mapping as the key. It means, that you need to have not 3 arrays but arrays for firstprivates, tofrom, to, from, alloc, etc. And all this processing must be in one place, in the class.

cchen updated this revision to Diff 228502.Nov 8 2019, 11:47 AM

Use implicit behavior as key for implicit map

ABataev added inline comments.Nov 8 2019, 12:13 PM
clang/lib/Parse/ParseOpenMP.cpp
2322–2323

Is this possible at all? I think it must an assertion rather this kind of trick.

clang/lib/Sema/SemaOpenMP.cpp
633

Cache the result of getDefaultDMIBAtLevel(Level, DMVC) in a var, do not call it several times.

657

Same, cache the result of the call

663

Seems to me, the code is not formatted. Use clang-format.

664

same here

2865

I would prefer to use OpenMPMapClauseKind type as a key here to prevent all those conversions of the default (none, default, unknown) mappings to real mappings.

3004–3005

Enclose this code into braces too, if the code in else branch is enclosed in braces the code in then branch also must be enclosed in braces.

3007

Better to use switches rather such kind of comparisons for enums.

3008–3009

Format the code.

cchen marked an inline comment as done.Nov 8 2019, 12:47 PM
cchen added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
2322–2323

This is definitely possible since the getOpenMPSimpleClauseType function in OpenMPKinds.cpp, parse defaultmap modifier and defaultmap kind in a same stringswitch statement, so for defaultmap(scalar, it will set the defaultmap modifier to be 0, however, the unknown is 3 (OMPC_DEFAULTMAP_MODIFIER_unknown == OMPC_DEFAULTMAP_unknown).

cchen updated this revision to Diff 228529.Nov 8 2019, 3:12 PM

clang-format all of my changes and fix based on feedback

ABataev added inline comments.Nov 11 2019, 7:56 AM
clang/lib/Parse/ParseOpenMP.cpp
2322–2323

Ok, I see. Then store the result in the temp var and change it there rather use Arg.back() = ....I still don't like this solution but the redesign is the different problem.

clang/lib/Sema/SemaOpenMP.cpp
673

Why DMIB_firstprivate is the default behavior? This is true only for scalars and pointers. Plus, this function is used in the context that does not match its name. Better to rename it to something like mustBeFirstprivate or something similar.

677

Same here

2822

const ValueDecl *

ABataev added inline comments.Nov 11 2019, 7:56 AM
clang/lib/Sema/SemaOpenMP.cpp
2839–2858

Why inner switch for the same variable DMIB?

4438

Use preincrement instead of postincrement.

4478

Use range-based loop here.

4479

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

16415–16438

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

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

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.Nov 11 2019, 9:21 AM
clang/lib/Sema/SemaOpenMP.cpp
4478

Better to use a new counter var here for ClauseKind

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

Fix based on feedback

ABataev added inline comments.Nov 11 2019, 1:29 PM
clang/lib/Sema/SemaOpenMP.cpp
1897–1899

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

2115–2117

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

16415–16438

What about this?

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

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.Nov 11 2019, 1:51 PM

Refactor by taking advantage of helper

ABataev added inline comments.Nov 12 2019, 8:57 AM
clang/lib/Parse/ParseOpenMP.cpp
2319–2326

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
3002–3004

Stack->mustBeFirstprivate(DMVC)?

4441

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

16415–16438

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.Nov 12 2019, 10:12 AM
cchen added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
2319–2326

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
16415–16438

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

ABataev added inline comments.Nov 12 2019, 10:27 AM
clang/lib/Parse/ParseOpenMP.cpp
2319–2326

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

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

Remove unnecessary enums and do some renaming

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

Static function

3092

true->/*Cond=*/true

4463

for (ArrayRef<Expr *> ImplicitMaps: ImplicitMaps)

16469

Not formatted

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

Fix based on feedback

cchen updated this revision to Diff 229119.Nov 13 2019, 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.Nov 13 2019, 9:13 AM
cchen updated this revision to Diff 229169.Nov 13 2019, 1:08 PM

Remove duplicate code in test

cchen marked an inline comment as done.Nov 13 2019, 1:11 PM
ABataev added inline comments.Nov 13 2019, 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.Nov 13 2019, 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.Nov 13 2019, 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.Nov 13 2019, 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.Nov 13 2019, 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.Nov 13 2019, 2:10 PM

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

ABataev added inline comments.Nov 14 2019, 8:05 AM
clang/lib/Parse/ParseOpenMP.cpp
2020–2034

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?

2823

Better to rename Cond to something like IsAggregateOrDeclareTarget

2847–2851
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.Nov 14 2019, 9:08 AM

Fix based on feedback

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

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.Nov 14 2019, 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.Nov 14 2019, 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.Nov 14 2019, 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.Nov 14 2019, 1:23 PM

Add test about declare target link/to

ABataev added inline comments.Nov 14 2019, 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.Nov 14 2019, 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.Nov 14 2019, 1:57 PM

Fix a silly bug in test

ABataev added inline comments.Nov 15 2019, 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.Nov 15 2019, 9:24 AM

Fix test

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

LG with a nit

clang/lib/Sema/SemaOpenMP.cpp
4462–4466

unsigned->int

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

Fix a wrong type

cchen added a comment.Nov 15 2019, 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.