This implements the default(firstprivate) clause as defined in OpenMP
Technical Report 8 (2.22.4).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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) |
Modifies clang-tidy to include the new clause, and changes the value of DSA_firstprivate.
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. |
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.
@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 | ||
---|---|---|
7234 | 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? |
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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
5497 | There are tests with expected-errors from failing to explicitly declare the data-sharing attributes for those variables. |
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.
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? |
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. |
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. |
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. |
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. |
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. |
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. |
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. |
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 |
| |
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. |
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. |
Necessary cleanups and adds a basic check for static local variables and static data members.
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. |
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. |
clang/lib/Parse/ParseOpenMP.cpp | ||
---|---|---|
2789 | This enum should be converted to int implicitly, no? |
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. |
clang/lib/Parse/ParseOpenMP.cpp | ||
---|---|---|
2789 | I see. Ok then, leave it as is |
Nit: -shared +firstprivate