Page MenuHomePhabricator

[OpenMP] Add firstprivate as a default data-sharing attribute to clang
ClosedPublic

Authored by atmnpatel on Mar 3 2020, 7:56 PM.

Details

Summary

This implements the default(firstprivate) clause as defined in OpenMP
Technical Report 8 (2.22.4).

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
atmnpatel updated this revision to Diff 254917.Apr 3 2020, 2:20 PM

Fixes clang-tidy check, and hopefully the other unrelated test failure too.

jdoerfert accepted this revision.Apr 8 2020, 3:03 PM

Apologies for my delay.

LGTM. Wait for @lebedev.ri though.

clang-tools-extra/docs/clang-tidy/checks/openmp-use-default-none.rst
56

Nit: -shared +firstprivate

clang/lib/Sema/SemaOpenMP.cpp
12979

@lebedev.ri This should address your issue, correct?

atmnpatel updated this revision to Diff 256152.Apr 8 2020, 5:53 PM

Addresses D77731 and fixes the comment.

atmnpatel marked an inline comment as done.Apr 8 2020, 5:55 PM
lebedev.ri resigned from this revision.Apr 8 2020, 11:11 PM

No further comments.

This revision is now accepted and ready to land.Apr 8 2020, 11:11 PM
ABataev added a subscriber: ABataev.Apr 9 2020, 5:16 AM
ABataev added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
1195

Need to add DVar.ImplicitDSALoc = Iter->DefaultAttrLoc;

3382–3384

I think just !VD->hasLocalStorage() should be enough here. Shall it work for static locals too or just for globals?

12973–12987

Better to turn it to switch

clang/test/OpenMP/parallel_master_codegen.cpp
145

Some extra work is required. The variable should not be captured by reference, must be captured by value. Also, a test with calling constructors/destructors is required.

149

This check may not work in some cases, better to rework it.

atmnpatel marked 3 inline comments as done.

Addresses inline comments.

atmnpatel marked an inline comment as done and an inline comment as not done.Apr 21 2020, 12:23 PM
atmnpatel added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
3382–3384

Just for globals.

clang/test/OpenMP/parallel_master_codegen.cpp
145

Sorry, I added a test with constructors/destructors with codegen. The variable capture discussion was had above and I'm pretty sure that the conclusion above (correct me if I'm wrong) was that its not ideal, but its fine for now and will be adjusted with the upcoming OMPIRBuilder.

ABataev added inline comments.Apr 21 2020, 12:31 PM
clang/lib/Sema/SemaOpenMP.cpp
3382–3384

What about static data members?

clang/test/OpenMP/parallel_master_codegen.cpp
145

It shall emit the correct code anyway. With default(firstprivate) and explicit firstprivate clause the codegen will be different and it may lead to an incompatibility with the existing libraries/object files.

atmnpatel marked 2 inline comments as done.Apr 21 2020, 12:43 PM
atmnpatel added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
3382–3384

The TR doesn't specify that static data members should inherit DSAs / have them explicitly defined.

clang/test/OpenMP/parallel_master_codegen.cpp
145

Ohh I see your concern now. Is the conclusion that I should just table this until something new pops up? I couldn't follow up on @fghanim's advice (sorry I've been on and off trying different things for weeks), it's beyond my capabilities.

atmnpatel marked 2 inline comments as not done.Apr 21 2020, 12:45 PM
ABataev added inline comments.Apr 21 2020, 1:07 PM
clang/lib/Sema/SemaOpenMP.cpp
3382–3384

What about non-static globals? Your current check works only for something like static int var;, but not int var;.

clang/test/OpenMP/parallel_master_codegen.cpp
145

You need to modify function Sema::isOpenMPCapturedByRef. There is a check for firstprivates in there, you need to add a check that the variable has no explicit data-sharing attributes and the default data sharing is set to firstprivate.

atmnpatel updated this revision to Diff 259122.Apr 21 2020, 4:11 PM

Add capture by value for scalars.

atmnpatel added inline comments.Apr 21 2020, 4:12 PM
clang/lib/Sema/SemaOpenMP.cpp
3382–3384

Sorry, I wasn't really clear with my earlier comment, I meant that just static global variables and static namespace variables need to have their DSA inherited/explicitly defined. So a global variable would become firstprivate implicitly.

atmnpatel added inline comments.Apr 21 2020, 4:14 PM
clang/lib/Sema/SemaOpenMP.cpp
2085–2092

@ABataev Is this what you mean?

ABataev added inline comments.May 11 2020, 1:52 PM
clang/lib/Sema/SemaOpenMP.cpp
39

Why do you need this here?

2085–2092

Did you try to compile it? What is VarsWithInheritedDSAType, where is it declared and defined? You just need to check for something like this:

DSAStack->getDefaultDSA() == DSA_firstprivate && Ty->isScalarType() && !DSAStack->hasExplicitDSA(D, [](OpenMPClauseKind K) {return K != OMPC_unknown;} && !DSAStack->isLoopControlVariable(D, Level).first

i.e. the default DSA is firstprivate, the type is scalar, no explicit DSA for this variable and the variable is not a loop control variable.

atmnpatel updated this revision to Diff 263326.EditedMay 11 2020, 5:52 PM

Fixed check for scalar types.

ABataev added inline comments.May 12 2020, 1:07 PM
clang/lib/Sema/SemaOpenMP.cpp
2088

/*NotLastprivate=*/true is not needed here. Also, could you move it under control of the if statement above?

2214–2216

Why do you need this check here? getTopDSA will return OMPC_firstprivate and one of the previous checks should be fired.

3382
  1. You need to check that the variable is not a loop control variable here.
  2. Seems to me you misinterpret the standard. According to the standard An object whose identifier is declared without the storage-class specifier _Thread_local, and either with external or internal linkage or with the storage-class specifier static, has static storage duration.. It means that non-static globals also must be affected since they have external linkage. So, you need to use getStorageDuration() function here and check that it returns SD_Static.
3383–3384

I think just CanonicalVD->getDeclContext()->isFileContext() must be enough here.

3389–3394

This code must be somewhere later, otherwise, you're skipping a lot of checks for the target-based directives. This must be checked and processed when we request implicit data sharing attributes with Stack->getImplicitDSA(VD, /*FromParent=*/false);

12937

Remove this empty line.

12974–12978

Usually, such checks are implemented in ParseOpenMP.cpp rather than in Sema.

atmnpatel updated this revision to Diff 263832.May 13 2020, 1:00 PM

Addresses inline comments.

Do you have the tests for static locals and static data members with default(firstprivate)?

clang/lib/Parse/ParseOpenMP.cpp
2789

No need for cast here.

2791

Remove this commented line of code.

2795

There should be return nullptr; after the Diag()

clang/lib/Sema/SemaOpenMP.cpp
3481–3482

Hmm, maybe move this check to getDSA()? If you do it, you can just modify the check on line 3322

if (DVar.CKind == OMPC_unknown && (Stack->getDefaultDSA() == DSA_none || (Stack->getDefaultDSA() == DSA_firstprivate && !Stack->isLoopControlVariable(VD).first)) &&
    isImplicitOrExplicitTaskingRegion(DKind) &&
    VarsWithInheritedDSA.count(VD) == 0) {
  VarsWithInheritedDSA[VD] = E;
  return;
}
3482–3487

Hmm, not sure that this check is needed here, the next if statement should handle it already, no? DVar.CKind is set to OMPC_firstprivate and the next check must work.

atmnpatel updated this revision to Diff 264365.May 15 2020, 3:00 PM

Necessary cleanups and adds a basic check for static local variables and static data members.

atmnpatel added inline comments.May 15 2020, 3:02 PM
clang/lib/Sema/SemaOpenMP.cpp
3481–3482

Didn't we move it this far down to avoid processing it before all of the target-directive relevant checks?

3482–3487

It doesn't work directly but I can simplify it further and merge them.

ABataev added inline comments.May 18 2020, 5:40 AM
clang/lib/Sema/SemaOpenMP.cpp
3479

You don't need to get the canonical decl here, VD already points to the canonical decl.

3481–3482

Yes, but seems to me I was wrong. The standard says "Specifying a variable in a map clause of an enclosed construct may cause an implicit reference to the variable in the enclosing construct. Such implicit references are also subject to the data-sharing attribute rules outlined in this section." If I interpret it correctly, it means that data-sharing attributes must be checked at first. Correct me if I wrong here.

Updated tests and includes changes made by @ABataev.

Ping @ABataev.

Rebase, please

atmnpatel updated this revision to Diff 275855.Jul 6 2020, 3:56 PM

Fixed tests.

ABataev accepted this revision.Jul 7 2020, 8:03 AM

LGTM with a nit.

clang/lib/Parse/ParseOpenMP.cpp
2789

Still no need for the cast

atmnpatel added inline comments.Jul 7 2020, 11:05 AM
clang/lib/Parse/ParseOpenMP.cpp
2789

Sorry, I saw that before and checked if I could remove it and I can't. Val.getValue().Type is an unsigned int and the other is an enum.

ABataev added inline comments.Jul 7 2020, 11:07 AM
clang/lib/Parse/ParseOpenMP.cpp
2789

This enum should be converted to int implicitly, no?

atmnpatel marked an inline comment as done.Jul 7 2020, 11:12 AM
atmnpatel added inline comments.
clang/lib/Parse/ParseOpenMP.cpp
2789

When we moved the definition of this enum over from clang to llvm, we converted them to strongly typed enums.

atmnpatel marked an inline comment as not done.Jul 7 2020, 11:12 AM
ABataev added inline comments.Jul 7 2020, 11:14 AM
clang/lib/Parse/ParseOpenMP.cpp
2789

I see. Ok then, leave it as is

This revision was automatically updated to reflect the committed changes.