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
1907

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
426–436

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

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

7276

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

Do you really need the default params values here?

clang/lib/Sema/SemaOpenMP.cpp
134

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

149

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?

623

Do you really need the static_cast here?

625

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

630

Do you really need a cast here?

656

Better to use switch here.

657–673

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

2928

Just isAnyPointerType()

2932–2933

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

2936

Just isAggregateType()?

2996–3003

Also must include DMIB_firstprivate.

16365

The code is not formatted.

16386

Better to use switch here too

16409

No default

clang/test/OpenMP/target_defaultmap_codegen.cpp
44

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
426–436

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
426–436

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

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
7245–7247

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.

8340

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

clang/lib/Sema/SemaOpenMP.cpp
149

Not done.

cchen marked 2 inline comments as done.Nov 4 2019, 9:34 PM
cchen added inline comments.
clang/lib/CodeGen/CGOpenMPRuntime.cpp
7245–7247

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
149

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
7245–7247

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

clang/lib/Sema/SemaOpenMP.cpp
149

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
1897–1898

Not done

2840

No need else if here , just if

2842

No need for else here, just return

3215–3216

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

4388

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

16457

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
1897–1898

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

4388

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
4388

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
4388

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
4388

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
4388

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
4388

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
621

auto -> to real type

627

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

2124–2125

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

3215

Revert back to ArrayRef

4458–4460

Use switch

16352
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16375
  1. Function names must start from verb.
  2. Functions must start from lower letter.
16378–16387

Just M != OMPC_DEFAULTMAP_MODIFIER_unknown and Kind != OMPC_DEFAULTMAP_unknown

16393

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
16378–16387

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
16378–16387

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
4458–4460

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
4458–4460

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
2343–2344

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

clang/lib/Sema/SemaOpenMP.cpp
643

Same, cache the result of the call

644

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

645

same here

656

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

2845

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.

3005–3006

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.

3009

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

3010–3011

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
2343–2344

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
2343–2344

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
654

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.

658

Same here

2837

const ValueDecl *

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

Why inner switch for the same variable DMIB?

4421

Use preincrement instead of postincrement.

4451

Use range-based loop here.

4452

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

16352–16375

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
4451

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
4451

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
1898–1903

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

2119–2132

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

16352–16375

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
16352–16375

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
2340–2341

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

Stack->mustBeFirstprivate(DMVC)?

4424

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

16352–16375

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
2340–2341

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
16352–16375

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
2340–2341

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
665

Static function

3088

true->/*Cond=*/true

4441

for (ArrayRef<Expr *> ImplicitMaps: ImplicitMaps)

16375

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
51

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
51

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
51

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
51

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
51

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
2049–2054

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
137

Kind is unused here, do you really need it?

2838

Better to rename Cond to something like IsAggregateOrDeclareTarget

2862–2866
if (Cond) {
  Kind = OMPC_MAP_tofrom;
  break;
}
llvm_unreachable("Unexpected defaultmap implicit behavior");
2933–2934

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
2862

Is this still true for decalare target to vars?

2933–2934

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
2933–2934

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
2932

Why extra parens in the condition?

2933–2934

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
4440

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.