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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 40961 Build 41109: arc lint + arc unit
Event Timeline
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
1905 | I somehow forgot about this, will fix this soon. |
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()? | |
3002–3009 | Also must include DMIB_firstprivate. | |
16417 | The code is not formatted. | |
16476 | Better to use switch here too | |
16499 | 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. |
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? |
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 |
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. |
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 |
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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
1894 | Not done | |
2824 | No need else if here , just if | |
2826 | No need for else here, just return | |
3221–3224 | Better to get each particular list via the DMVC_ key passed as parameter | |
4404 | OMPC_MAP_tofrom must be returned only for DMIB_tofrom, for others, it must be llvm_unreachable(....) | |
16547 | Just create 2 variables and initialize them. Call setDefault... only once. |
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. | |
4404 | 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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4404 | Then this function is not correct because for scalars default is not tofrom. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4404 | 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). |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4404 | You need to add extra checks for scalars and pointers in this function. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4404 | 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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4404 | Again, it means that this function is not correct in all cases and should be reworked somehow or deleted. |
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. | |
3221–3223 | Revert back to ArrayRef | |
4483–4485 | Use switch | |
16413 |
| |
16436 |
| |
16469–16478 | Just M != OMPC_DEFAULTMAP_MODIFIER_unknown and Kind != OMPC_DEFAULTMAP_unknown | |
16484 | Why need to add a new Loc var here? Use the required KindLoc\MLoc directly. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
16469–16478 | 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? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
16469–16478 | 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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4483–4485 | 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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4483–4485 | 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. |
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. | |
3002–3003 | 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. | |
3005 | Better to use switches rather such kind of comparisons for enums. | |
3006–3007 | Format the code. |
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). |
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 * |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
2838–2857 | Why inner switch for the same variable DMIB? | |
4436 | Use preincrement instead of postincrement. | |
4476 | Use range-based loop here. | |
4477 | Simplify inner complexity here by using continue; if the condition is true. | |
16413–16436 | Do we need types DefaultMapImplicitBehavior and DefaultMapVariableCategory if the map 1 to 1 to OpenMPDefaultmapClauseModifier and OpenMPDefaultmapClauseKind? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4476 | 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. |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
4476 | Better to use a new counter var here for ClauseKind |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
1896–1898 | Just DSAStack->isDefaultmapCapturedByRef(Level, getVariableCategoryFromDecl(D)) | |
2114–2116 | Just DSAStack->mustBeFirstprivateAtLevel(NewLevel, getVariableCategoryFromDecl(D)) | |
16413–16436 | What about this? |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
16413–16436 | 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. |
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 | ||
3000–3002 | Stack->mustBeFirstprivate(DMVC)? | |
4439 | ImplicitMaps[I].append(ImplicitMap.begin(), ImplicitMap.end()); | |
16413–16436 | 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? |
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) ||.... | |
clang/lib/Sema/SemaOpenMP.cpp | ||
16413–16436 | Got it, I'll use OpenMPDefaultmapClauseModifier and OpenMPDefaultmapClauseKind instead. Thanks |
clang/lib/Parse/ParseOpenMP.cpp | ||
---|---|---|
2313–2320 | You will just commit the second patch at first and then just rebase this patch. |
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? |
clang/test/OpenMP/target_defaultmap_messages.cpp | ||
---|---|---|
49 | Same test case must exist for OpenMP 4.5 |
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. |
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 |
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? |
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 |
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. |
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. |
Tests for codegen of the declare target to|link vars with the new defaultmap clauses?
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. |
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. |
clang/test/OpenMP/target_defaultmap_codegen.cpp | ||
---|---|---|
1545–1546 | Don't understand how does it test mapping of the declare target link vars. |
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!
is_device_ptr, not is_devise_ptr