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
1905

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

129

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?

602

Do you really need the static_cast here?

604

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

609

Do you really need a cast here?

674

Better to use switch here.

675–691

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()?

3001–3008

Also must include DMIB_firstprivate.

16416

The code is not formatted.

16475

Better to use switch here too

16498

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
129

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
129

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
129

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
1894

Not done

2824

No need else if here , just if

2826

No need for else here, just return

3220–3223

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

4403

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

16546

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
1894

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

4403

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
4403

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
4403

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
4403

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
4403

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
4403

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
600

auto -> to real type

606

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

2119–2120

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

3220–3222

Revert back to ArrayRef

4482–4484

Use switch

16412
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16435
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16468–16477

Just M != OMPC_DEFAULTMAP_MODIFIER_unknown and Kind != OMPC_DEFAULTMAP_unknown

16483

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
16468–16477

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
16468–16477

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
4482–4484

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
4482–4484

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
2316–2317

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

clang/lib/Sema/SemaOpenMP.cpp
632

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

656

Same, cache the result of the call

662

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

663

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.

3001–3002

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.

3004

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

3005–3006

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
2316–2317

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
2316–2317

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
672

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.

676

Same here

2821

const ValueDecl *

ABataev added inline comments.Nov 11 2019, 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.Nov 11 2019, 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.Nov 11 2019, 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.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
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.Nov 11 2019, 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.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
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.Nov 12 2019, 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.Nov 12 2019, 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.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
641

Static function

3089

true->/*Cond=*/true

4460

for (ArrayRef<Expr *> ImplicitMaps: ImplicitMaps)

16466

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
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.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
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.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
1544–1545

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
4459–4463

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.