This is an archive of the discontinued LLVM Phabricator instance.

[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

atmnpatel created this revision.Mar 3 2020, 7:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 3 2020, 7:56 PM
atmnpatel updated this revision to Diff 248098.Mar 3 2020, 9:05 PM

Fixes typos that causes certain unit tests to fail.

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

clang/lib/Sema/SemaOpenMP.cpp
58

why 0b11 ? shouldn't this be 0b100 (1 << 2)

atmnpatel updated this revision to Diff 248310.Mar 4 2020, 1:59 PM

Modifies clang-tidy to include the new clause, and changes the value of DSA_firstprivate.

jdoerfert added inline comments.Mar 5 2020, 12:02 PM
clang/test/OpenMP/parallel_master_codegen.cpp
143

This does not look like firstprivate is actually doing anything here. Could you please confirm that the IR is the same for default(shared) and default(firstprivate)? If so, we need to figure out why the variable is not marked firstprivate.

atmnpatel updated this revision to Diff 249570.Mar 11 2020, 2:27 AM

Modified the unit test for CodeGen of default(firstprivate) to more accurately reflect the IR.

From the perspective of the AST, the variables that are firstprivate from a default clause are managed in a way that is similar to how variables in a task are marked firstprivate. This is responsible for the discrepancy between the IR for 'default(firstprivate)' and 'default(none) firstprivate(...)'. When a firstprivate clause is handled, it has an explicit list where it stores all private copies that it has constructed for each variable because it has access to an explicit list of variables for which it pre-emptively creates new copies for. For a default clause, if a new copy of a variable needs to be created in a manner idential to 'firstprivate(...)', it would require a complete restructuring of how the default clause is handled because before the constructor for the default clause can be called, we would need to iterate over the contents of the region in order to find references to variables that would need to become firstprivate to create and store copies.

The way that I see it, is that this would be require a much more substantial change (than the current proposed implementation) to the handling of the default clause - the construction of the default clause node in the AST would need to be postponed until after the contents of the openmp region had been placed into an AST and the nodes visited in order to find the relevant variables. It is my belief that this would lead tto a more convoluted implementation whose behavior is identical to the current one.

I can confirm that in the IR it does not behave as an explicit firstprivate clause for the variables it marks as firstprivate but I don't understand enough to know why this causes a functional difference because the variables are handled as firstprivate in the same way that task handles them.

Modified the unit test for CodeGen of default(firstprivate) to more accurately reflect the IR.

From the perspective of the AST, the variables that are firstprivate from a default clause are managed in a way that is similar to how variables in a task are marked firstprivate. This is responsible for the discrepancy between the IR for 'default(firstprivate)' and 'default(none) firstprivate(...)'. When a firstprivate clause is handled, it has an explicit list where it stores all private copies that it has constructed for each variable because it has access to an explicit list of variables for which it pre-emptively creates new copies for. For a default clause, if a new copy of a variable needs to be created in a manner idential to 'firstprivate(...)', it would require a complete restructuring of how the default clause is handled because before the constructor for the default clause can be called, we would need to iterate over the contents of the region in order to find references to variables that would need to become firstprivate to create and store copies.

The way that I see it, is that this would be require a much more substantial change (than the current proposed implementation) to the handling of the default clause - the construction of the default clause node in the AST would need to be postponed until after the contents of the openmp region had been placed into an AST and the nodes visited in order to find the relevant variables. It is my belief that this would lead tto a more convoluted implementation whose behavior is identical to the current one.

I can confirm that in the IR it does not behave as an explicit firstprivate clause for the variables it marks as firstprivate but I don't understand enough to know why this causes a functional difference because the variables are handled as firstprivate in the same way that task handles them.

I think we can make default(firsprivate) and default(private) work for the OpenMPIRBuilder path much easier. I was hoping we could get it before but I get the complications you describe.

@fghanim where are we with the privatization in the new codegen?

Done with private firstprivate copyin - working on modifying the clang test, and fixing some bugs.

I think this is functionally correct even if we pass the pointer for now as it is properly "privatized" in the callee.

I will go over this once more (after some sleep) but I think it is all good.

Once @fghanim puts the new code path on phab we need to ensure it works well with this addition.

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

@atmnpatel This is still an open issue, correct?

Could you also mention again if and how we could implement default(private), or what problems you envision?

Yep, it is still an open issue. I am actually unsure of how to do that, and I'd love some pointers.

As far as default(private) goes, I am also unsure of how to do it without retaining the prior values of the variables. I will take a look over this weekend and see if I can find a workaround. All I had so far was that maybe we could push it alongside the implicitly firstprivate variables but reassign it to a null/default value before/after doing so, but I'm not certain that it's an ideal solution.

@jdoerfert To answer your question as to why it is being codegened as shared, codegen doesn't handle default clause- at least I didn't come across such a thing- and if you think about it, it shouldn't need to. You either have none or shared:

  • none is largly to throw diag errors - so clang either never leaves sema because it threw an error, or there are no errors, which suggests that the programmer has already listed all captured variables as other clauses. - either way, codegen doesn't need to handle it.
  • shared is the default anyway, and codegen already makes any captured variableby the outlined function not "claimed" by any other privatization clause, shared - it just passes it as an arg. to the outlined function.

and Since @atmnpatel didn't add any new codegen for the default{firstprivate} case, codegen defaulted to shared - at least that's what I think happened.

@atmnpatel The way privatization works currently in codegen, you go over every clause, find the list of variables for that clause, find some relevant info about the variable along with the proper way to initialize the variable (i.e. what constructor to use), and add it to list of all variables to be privatized, once done with all privatization clauses, you do codegen for privatization for all of them in one go. The exception to this is copyin.
So what you may want to do is to handle the default clause in codegen. if it's none or shared, you ignore it. if it is firstprivate or private, find the captured variables of the soon to be outlined function that have not been captured by any other privatization clause, then figure a way to pass it to codegenfunction::emitOMPFirstprivateClause or codegenfunction::emitPrivateClause. Alternatively, find this list while building the AST, and add them as default captured variables, which will make codegen much easier. You are already checking for them to throw the none diag error anyway.

clang/include/clang/ASTMatchers/ASTMatchers.h
7214

fix comment: ... has `firstprivate` kind ...

clang/lib/Sema/SemaOpenMP.cpp
5379–5380

fix the comments to say firstprivate as well (here and elsewhere)

5497

Why is firstprivate throwing this error? isn't the purpose of specifying it as default is if a variable is not specified as anything, then it is automatically handled as firstprivate? or am I misunderstanding something?

atmnpatel added inline comments.Mar 14 2020, 7:50 PM
clang/lib/Sema/SemaOpenMP.cpp
5497

My understanding is that if that line isn't there then errors won't be thrown in the special cases where firstprivate explicitly requires a data sharing attribute to be specified - such as for static variable within a namespace/global scope as per the C/C++ restrictions in the technical report.

I'm satisfied with this, @fghanim wdyt?

clang/lib/Sema/SemaOpenMP.cpp
5497

Do we have a test for that or can we add one, please. (If you remove this and an error we expect in the existing tests is not shown anymore that is sufficient.)

lebedev.ri requested changes to this revision.Mar 20 2020, 4:19 AM

IIUC default(firstprivate) is being added in openmp-5.1,
it is not in openmp-4.5, so it should not be accepted
in pre-openmp-5.1-mode (i.e. it should be diagnosed)

This revision now requires changes to proceed.Mar 20 2020, 4:19 AM

My comments were nits so.

LGTM

atmnpatel marked 3 inline comments as done.Mar 21 2020, 9:27 AM
atmnpatel added inline comments.
clang/lib/Sema/SemaOpenMP.cpp
5497

There are tests with expected-errors from failing to explicitly declare the data-sharing attributes for those variables.

atmnpatel updated this revision to Diff 251847.Mar 21 2020, 9:27 AM

Comment fixes.

Fixed linter error.

Did you add the warning @lebedev.ri mentioned? (@lebedev.ri I assume/hope a warning is sufficient here?)

Did you add the warning @lebedev.ri mentioned? (@lebedev.ri I assume/hope a warning is sufficient here?)

It should be handled consistently with how other newer openmp features are handled with older openmp version specified.

In D75591#1936152, @lebedev.ri wrote:
It should be handled consistently with how other newer openmp features are handled with older openmp version specified.

In terms of consistency, the newer openmp features are handled as errors thrown, not as warnings. In this particular case however, it might make more sense to keep this as a warning rather than an error because its functionality doesn't exactly rely on a particular openmp version. I have it written out as an error now, but it doesn't quite sit right with me as an error yet.

atmnpatel updated this revision to Diff 253463.Mar 29 2020, 4:02 PM

Added error based on OpenMP version.

Second try at generating a patch file.

atmnpatel updated this revision to Diff 254379.Apr 1 2020, 5:41 PM

Fixes to tests.

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
2788

No need for cast here.

2790

Remove this commented line of code.

2794

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
2788

Still no need for the cast

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

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
2788

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
2788

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
2788

I see. Ok then, leave it as is

This revision was automatically updated to reflect the committed changes.