[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69204#1747941 , @cchen wrote:

> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4459
 }
-if (!ImplicitMaps.empty()) {
+unsigned ClauseKindCnt = -1;
+for (ArrayRef ImplicitMap : ImplicitMaps) {

`unsigned`->`int`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_codegen.cpp:1544-1545
+  // CK26: [[ONE:%.+]] = load float, float* [[ZERO]]
+#pragma omp target defaultmap(none:aggregate) map(A)
+  compute(A);
+}

Don't understand how does it test mapping of the declare target link vars.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_codegen.cpp:1538-1546
+void declare_target_link(float A)
+{
+  // CK26: [[AADDR:%.+]] = alloca float*
+  // CK26: store float* [[A:%.+]], float** [[AADDR]]
+  // CK26: [[ZERO:%.+]] = load float*, float** [[AADDR]]
+  // CK26: [[ONE:%.+]] = load float, float* [[ZERO]]
+#pragma omp target defaultmap(none:aggregate) map(A)

ABataev wrote:
> What does this test check? It is not the test for declare target link. Also, 
> A is calar and defaultmap is for aggregates.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_codegen.cpp:1538-1546
+void declare_target_link(float A)
+{
+  // CK26: [[AADDR:%.+]] = alloca float*
+  // CK26: store float* [[A:%.+]], float** [[AADDR]]
+  // CK26: [[ZERO:%.+]] = load float*, float** [[AADDR]]
+  // CK26: [[ONE:%.+]] = load float, float* [[ZERO]]
+#pragma omp target defaultmap(none:aggregate) map(A)

What does this test check? It is not the test for declare target link. Also, A 
is calar and defaultmap is for aggregates.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Tests for codegen of the declare target to|link vars with the new defaultmap 
clauses?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2953-2954
+(!(isOpenMPTargetExecutionDirective(DKind) && Res &&
+   (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+*Res == OMPDeclareTargetDeclAttr::MT_To {
+  // Only check for data-mapping attribute and is_device_ptr here

cchen wrote:
> ABataev wrote:
> > ABataev wrote:
> > > 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.
> > Do we need to map `declare target to` vars at all?
> 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?
Missed `!` in the condition. Why we check for 
`!isOpenMPTargetExecutionDirective(DKind)`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2952
+VarsWithInheritedDSA.count(VD) == 0 &&
+(!(isOpenMPTargetExecutionDirective(DKind) && Res))) {
+  // Only check for data-mapping attribute and is_device_ptr here

Why extra parens in the condition?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2953-2954
+(!(isOpenMPTargetExecutionDirective(DKind) && Res &&
+   (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+*Res == OMPDeclareTargetDeclAttr::MT_To {
+  // Only check for data-mapping attribute and is_device_ptr here

ABataev wrote:
> ABataev wrote:
> > 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.
> Do we need to map `declare target to` vars at all?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2953-2954
+(!(isOpenMPTargetExecutionDirective(DKind) && Res &&
+   (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+*Res == OMPDeclareTargetDeclAttr::MT_To {
+  // Only check for data-mapping attribute and is_device_ptr here

ABataev wrote:
> 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.
Do we need to map `declare target to` vars at all?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2846
+// 1. the implicit behavior for aggregate is tofrom
+// 2. it's a declare target link/to
+if (IsAggregateOrDeclareTarget) {

Is this still true for `decalare target to` vars?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2020-2034
 if (!FirstClause) {
   Diag(Tok, diag::err_omp_more_one_clause)
   << getOpenMPDirectiveName(DKind) << getOpenMPClauseName(CKind) << 0;
   ErrorFound = true;
 }
+Clause = ParseOpenMPSingleExprWithArgClause(CKind, WrongDirective);
+break;

I think better just to modify the condition in the original `if`:
```
if ((getLangOpts().OpenMP < 50 || CKind != OMPC_defaultmap) && !FirstClause)
```



Comment at: clang/lib/Sema/SemaOpenMP.cpp:118
+DefaultmapInfo(OpenMPDefaultmapClauseModifier M,
+   OpenMPDefaultmapClauseKind Kind, SourceLocation Loc)
+: ImplicitBehavior(M), SLoc(Loc) {}

`Kind` is unused here, do you really need it?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2823
+static OpenMPMapClauseKind
+getMapClauseKindFromModifier(OpenMPDefaultmapClauseModifier M, bool Cond) {
+  OpenMPMapClauseKind Kind = OMPC_MAP_unknown;

Better to rename `Cond` to something like `IsAggregateOrDeclareTarget`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2847-2851
+if (Cond)
+  Kind = OMPC_MAP_tofrom;
+else
+  llvm_unreachable("Unexpected defaultmap implicit behavior");
+break;

```
if (Cond) {
  Kind = OMPC_MAP_tofrom;
  break;
}
llvm_unreachable("Unexpected defaultmap implicit behavior");
```



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2953-2954
+(!(isOpenMPTargetExecutionDirective(DKind) && Res &&
+   (*Res == OMPDeclareTargetDeclAttr::MT_Link ||
+*Res == OMPDeclareTargetDeclAttr::MT_To {
+  // Only check for data-mapping attribute and is_device_ptr here

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:49
+
+  #pragma omp target defaultmap(tofrom:scalar) defaultmap(to:scalar) // 
expected-error {{at most one defaultmap clause for each variable-category can 
appear on the directive}}
+  foo();

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > Same test case must exist for OpenMP 4.5
> > > 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.
> > We don't have such test for OpenMP 4.5 and it would be good to have it
> 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?
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:49
+
+  #pragma omp target defaultmap(tofrom:scalar) defaultmap(to:scalar) // 
expected-error {{at most one defaultmap clause for each variable-category can 
appear on the directive}}
+  foo();

cchen wrote:
> ABataev wrote:
> > Same test case must exist for OpenMP 4.5
> 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.
We don't have such test for OpenMP 4.5 and it would be good to have it


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:49
+
+  #pragma omp target defaultmap(tofrom:scalar) defaultmap(to:scalar) // 
expected-error {{at most one defaultmap clause for each variable-category can 
appear on the directive}}
+  foo();

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > Same test case must exist for OpenMP 4.5
> > 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.
> We don't have such test for OpenMP 4.5 and it would be good to have it
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:49
+
+  #pragma omp target defaultmap(tofrom:scalar) defaultmap(to:scalar) // 
expected-error {{at most one defaultmap clause for each variable-category can 
appear on the directive}}
+  foo();

ABataev wrote:
> Same test case must exist for OpenMP 4.5
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:49
+
+  #pragma omp target defaultmap(tofrom:scalar) defaultmap(to:scalar) // 
expected-error {{at most one defaultmap clause for each variable-category can 
appear on the directive}}
+  foo();

Same test case must exist for OpenMP 4.5


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

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.




Comment at: clang/test/OpenMP/target_defaultmap_messages.cpp:1
-// RUN: %clang_cc1 -verify -fopenmp %s -Wuninitialized
 

What about the testing for the previous version of OpenMP, 45?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:642
+  }
+  bool mustBeFirstprivateBase(OpenMPDefaultmapClauseModifier M,
+  OpenMPDefaultmapClauseKind Kind) const {

Static function



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3092
+Stack->getDefaultmapModifier(OMPC_DEFAULTMAP_aggregate);
+OpenMPMapClauseKind Kind = getMapClauseKindFromModifier(Modifier, 
true);
+ImplicitMap[Kind].emplace_back(E);

`true`->`/*Cond=*/true`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4462
+unsigned ClauseKindCnt = -1;
+for (const auto  : ImplicitMaps) {
+  ++ClauseKindCnt;

`for (ArrayRef ImplicitMaps: ImplicitMaps)`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16420
+if (M != OMPC_DEFAULTMAP_MODIFIER_tofrom ||
+  Kind != OMPC_DEFAULTMAP_scalar) {
+  std::string Value;

Not formatted


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2339-2345
+unsigned Modifier = getOpenMPSimpleClauseType(
+Kind, Tok.isAnnotation() ? "" : PP.getSpelling(Tok));
+// Set defaultmap modifier to unknown if it is either scalar, aggregate, or
+// pointer
+if (Modifier < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Modifier = OMPC_DEFAULTMAP_MODIFIER_unknown;
+Arg.push_back(Modifier);

cchen wrote:
> ABataev wrote:
> > I believe this problem exists in the original code? If so, better to split 
> > this patch and commit this fix in the separate patch.
> 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?
You will just commit the second patch at first and then just rebase this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2339-2345
+unsigned Modifier = getOpenMPSimpleClauseType(
+Kind, Tok.isAnnotation() ? "" : PP.getSpelling(Tok));
+// Set defaultmap modifier to unknown if it is either scalar, aggregate, or
+// pointer
+if (Modifier < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Modifier = OMPC_DEFAULTMAP_MODIFIER_unknown;
+Arg.push_back(Modifier);

ABataev wrote:
> I believe this problem exists in the original code? If so, better to split 
> this patch and commit this fix in the separate patch.
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?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+return DMIB_to;

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > ABataev wrote:
> > > > Do we need types `DefaultMapImplicitBehavior` and 
> > > > `DefaultMapVariableCategory` if the map 1 to 1 to 
> > > > `OpenMPDefaultmapClauseModifier` and `OpenMPDefaultmapClauseKind`?
> > > What about this?
> > 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.
> 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?
Got it, I'll use OpenMPDefaultmapClauseModifier and OpenMPDefaultmapClauseKind 
instead. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2339-2345
+unsigned Modifier = getOpenMPSimpleClauseType(
+Kind, Tok.isAnnotation() ? "" : PP.getSpelling(Tok));
+// Set defaultmap modifier to unknown if it is either scalar, aggregate, or
+// pointer
+if (Modifier < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Modifier = OMPC_DEFAULTMAP_MODIFIER_unknown;
+Arg.push_back(Modifier);

I believe this problem exists in the original code? If so, better to split this 
patch and commit this fix in the separate patch.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+return DMIB_to;

cchen wrote:
> ABataev wrote:
> > ABataev wrote:
> > > Do we need types `DefaultMapImplicitBehavior` and 
> > > `DefaultMapVariableCategory` if the map 1 to 1 to 
> > > `OpenMPDefaultmapClauseModifier` and `OpenMPDefaultmapClauseKind`?
> > What about this?
> 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.
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?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3013-3020
+  IsFirstprivate = IsFirstprivate ||
+   (((DMVC == DMVC_pointer &&
+  Stack->mustBeFirstprivate(DMVC_pointer)) ||
+ (DMVC == DMVC_scalar &&
+  Stack->mustBeFirstprivate(DMVC_scalar)) ||
+ (DMVC == DMVC_aggregate &&
+  Stack->mustBeFirstprivate(DMVC_aggregate))) &&

`Stack->mustBeFirstprivate(DMVC)`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4448
+  
DSAChecker.getImplicitMap(static_cast(I));
+  for (const auto  : ImplicitMap)
+ImplicitMaps[I].emplace_back(M);

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+return DMIB_to;

ABataev wrote:
> Do we need types `DefaultMapImplicitBehavior` and 
> `DefaultMapVariableCategory` if the map 1 to 1 to 
> `OpenMPDefaultmapClauseModifier` and `OpenMPDefaultmapClauseKind`?
What about this?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1901-1904
+  (Ty->isScalarType() && !Ty->isPointerType() &&
+   DSAStack->isDefaultmapCapturedByRef(Level, DMVC_scalar)) ||
+  (Ty->isPointerType() &&
+   DSAStack->isDefaultmapCapturedByRef(Level, DMVC_pointer)) ||

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2121-2125
+  if ((D->getType()->isScalarType() && !D->getType()->isPointerType() &&
+   DSAStack->mustBeFirstprivateAtLevel(NewLevel, DMVC_scalar)) ||
+  (D->getType()->isPointerType() &&
+   DSAStack->mustBeFirstprivateAtLevel(NewLevel, DMVC_pointer)) ||
+  DSAStack->mustBeFirstprivateAtLevel(NewLevel, DMVC_aggregate))

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+return DMIB_to;

ABataev wrote:
> ABataev wrote:
> > Do we need types `DefaultMapImplicitBehavior` and 
> > `DefaultMapVariableCategory` if the map 1 to 1 to 
> > `OpenMPDefaultmapClauseModifier` and `OpenMPDefaultmapClauseKind`?
> What about this?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4476
 }
-if (!ImplicitMaps.empty()) {
-  CXXScopeSpec MapperIdScopeSpec;
-  DeclarationNameInfo MapperId;
-  if (OMPClause *Implicit = ActOnOpenMPMapClause(
-  llvm::None, llvm::None, MapperIdScopeSpec, MapperId,
-  OMPC_MAP_tofrom, /*IsMapTypeImplicit=*/true, SourceLocation(),
-  SourceLocation(), ImplicitMaps, OMPVarListLocTy())) {
-ClausesWithImplicit.emplace_back(Implicit);
-ErrorFound |=
-cast(Implicit)->varlist_size() != 
ImplicitMaps.size();
-  } else {
-ErrorFound = true;
+for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+  if (!ImplicitMaps[I].empty()) {

cchen wrote:
> ABataev wrote:
> > Use range-based loop here.
> 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.
Better to use a new counter var here for ClauseKind


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4476
 }
-if (!ImplicitMaps.empty()) {
-  CXXScopeSpec MapperIdScopeSpec;
-  DeclarationNameInfo MapperId;
-  if (OMPClause *Implicit = ActOnOpenMPMapClause(
-  llvm::None, llvm::None, MapperIdScopeSpec, MapperId,
-  OMPC_MAP_tofrom, /*IsMapTypeImplicit=*/true, SourceLocation(),
-  SourceLocation(), ImplicitMaps, OMPVarListLocTy())) {
-ClausesWithImplicit.emplace_back(Implicit);
-ErrorFound |=
-cast(Implicit)->varlist_size() != 
ImplicitMaps.size();
-  } else {
-ErrorFound = true;
+for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+  if (!ImplicitMaps[I].empty()) {

ABataev wrote:
> Use range-based loop here.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2343-2344
+// pointer
+if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
 KLoc.push_back(Tok.getLocation());

cchen wrote:
> ABataev wrote:
> > Is this possible at all? I think it must an assertion rather this kind of 
> > trick.
> 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).
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.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:659
+DefaultMapImplicitBehavior DMIB = getDefaultDMIB(DMVC);
+return (DMIB == DMIB_unspecified) || (DMIB == DMIB_firstprivate) ||
+   (DMIB == DMIB_default);

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.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:663
+  bool
+  isDefaultmapDefaultBehaviorAtLevel(unsigned Level,
+ DefaultMapVariableCategory DMVC) const {

Same here



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2824
 
+static DefaultMapVariableCategory getVariableCategoryFromDecl(ValueDecl *VD) {
+  if (VD->getType().getNonReferenceType()->isAnyPointerType())

`const ValueDecl *`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2841-2860
+switch (DMIB) {
+case DMIB_alloc:
+  Kind = OMPC_MAP_alloc;
+  break;
+case DMIB_to:
+  Kind = OMPC_MAP_to;
+  break;

Why inner switch for the same variable `DMIB`?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4451
+SmallVector ImplicitMaps[OMPC_MAP_delete];
+for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+  ArrayRef ImplicitMap =

Use preincrement instead of postincrement.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4476
 }
-if (!ImplicitMaps.empty()) {
-  CXXScopeSpec MapperIdScopeSpec;
-  DeclarationNameInfo MapperId;
-  if (OMPClause *Implicit = ActOnOpenMPMapClause(
-  llvm::None, llvm::None, MapperIdScopeSpec, MapperId,
-  OMPC_MAP_tofrom, /*IsMapTypeImplicit=*/true, SourceLocation(),
-  SourceLocation(), ImplicitMaps, OMPVarListLocTy())) {
-ClausesWithImplicit.emplace_back(Implicit);
-ErrorFound |=
-cast(Implicit)->varlist_size() != 
ImplicitMaps.size();
-  } else {
-ErrorFound = true;
+for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+  if (!ImplicitMaps[I].empty()) {

Use range-based loop here.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4477
+for (unsigned I = 0; I < OMPC_MAP_delete; I++) {
+  if (!ImplicitMaps[I].empty()) {
+CXXScopeSpec MapperIdScopeSpec;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16393-16416
+static DefaultMapImplicitBehavior
+getImplicitBehaviorFromModifier(OpenMPDefaultmapClauseModifier M) {
+  switch (M) {
+  case OMPC_DEFAULTMAP_MODIFIER_alloc:
+return DMIB_alloc;
+  case OMPC_DEFAULTMAP_MODIFIER_to:
+return DMIB_to;

Do we need types `DefaultMapImplicitBehavior` and `DefaultMapVariableCategory` 
if the map 1 to 1 to `OpenMPDefaultmapClauseModifier` and 
`OpenMPDefaultmapClauseKind`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2343-2344
+// pointer
+if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
 KLoc.push_back(Tok.getLocation());

ABataev wrote:
> Is this possible at all? I think it must an assertion rather this kind of 
> trick.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:2343-2344
+// pointer
+if (Arg.back() < OMPC_DEFAULTMAP_MODIFIER_unknown)
+  Arg.back() = OMPC_DEFAULTMAP_MODIFIER_unknown;
 KLoc.push_back(Tok.getLocation());

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:651
+if (DMVC == DMVC_scalar || DMVC == DMVC_pointer) {
+  return (getDefaultDMIBAtLevel(Level, DMVC) ==
+DMIB_alloc) ||

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:663
+  bool isDefaultmapDefaultBehavior(DefaultMapVariableCategory DMVC) const {
+return (getDefaultDMIB(DMVC) == DMIB_unspecified) ||
+   (getDefaultDMIB(DMVC) == DMIB_firstprivate) ||

Same, cache the result of the call



Comment at: clang/lib/Sema/SemaOpenMP.cpp:668
+  bool isDefaultmapDefaultBehaviorAtLevel(unsigned Level,
+   DefaultMapVariableCategory DMVC) const {
+return (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_unspecified) ||

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:669
+   DefaultMapVariableCategory DMVC) const {
+return (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_unspecified) ||
+   (getDefaultDMIBAtLevel(Level, DMVC) == DMIB_firstprivate) ||

same here



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2843
   llvm::SmallVector ImplicitFirstprivate;
-  llvm::SmallVector ImplicitMap;
+  llvm::SmallVector ImplicitMap[DMIB_none];
   Sema::VarsWithInheritedDSAType VarsWithInheritedDSA;

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.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2987
   if (IsFirstprivate)
 ImplicitFirstprivate.emplace_back(E);
+  else {

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.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2990
+DefaultMapImplicitBehavior DMIB = Stack->getDefaultDMIB(DMVC);
+if (DMIB >= DMIB_none) {
+  if (DMVC == DMVC_aggregate || Res) DMIB = DMIB_tofrom;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2991-2992
+if (DMIB >= DMIB_none) {
+  if (DMVC == DMVC_aggregate || Res) DMIB = DMIB_tofrom;
+  else llvm_unreachable("Unexpected defaultmap implicit behavior");
+}

Format the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4435-4437
+if (DMIB == DMIB_alloc) Kind = OMPC_MAP_alloc;
+else if (DMIB == DMIB_to) Kind = OMPC_MAP_to;
+else if (DMIB == DMIB_from) Kind = OMPC_MAP_from;

cchen wrote:
> ABataev wrote:
> > Use `switch`
> 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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4435-4437
+if (DMIB == DMIB_alloc) Kind = OMPC_MAP_alloc;
+else if (DMIB == DMIB_to) Kind = OMPC_MAP_to;
+else if (DMIB == DMIB_from) Kind = OMPC_MAP_from;

ABataev wrote:
> Use `switch`
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16411-16420
+bool isDefaultmapModifier = (M == OMPC_DEFAULTMAP_MODIFIER_alloc) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_to) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_from) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_tofrom) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_firstprivate) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_default) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_none);

cchen wrote:
> ABataev wrote:
> > Just `M != OMPC_DEFAULTMAP_MODIFIER_unknown` and `Kind != 
> > OMPC_DEFAULTMAP_unknown`
> 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?
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16411-16420
+bool isDefaultmapModifier = (M == OMPC_DEFAULTMAP_MODIFIER_alloc) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_to) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_from) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_tofrom) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_firstprivate) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_default) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_none);

ABataev wrote:
> Just `M != OMPC_DEFAULTMAP_MODIFIER_unknown` and `Kind != 
> OMPC_DEFAULTMAP_unknown`
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:620
+ SourceLocation Loc) {
+auto  = getTopOfStack().DefaultmapMap[DMVC];
+DefaultmapInfo.ImplicitBehavior = DMIB;

`auto` -> to real type



Comment at: clang/lib/Sema/SemaOpenMP.cpp:626
+  bool hasSetDefaultmapCategory(OpenMPDefaultmapClauseKind VariableCategory) {
+int VC = static_cast(VariableCategory);
+return getTopOfStack().DefaultmapMap[VC].ImplicitBehavior != 
DMIB_unspecified;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2120-2121
+isDefaultmapDefaultBehaviorAtLevel(NewLevel, DMVC_pointer)) ||
+  (DSAStack->
+isDefaultmapDefaultBehaviorAtLevel(NewLevel, DMVC_aggregate)))
 OMPC = OMPC_firstprivate;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3198
   }
-  ArrayRef getImplicitMap() const { return ImplicitMap; }
+  llvm::SmallVector
+getImplicitMap(DefaultMapVariableCategory DMVC) const {

Revert back to `ArrayRef`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4435-4437
+if (DMIB == DMIB_alloc) Kind = OMPC_MAP_alloc;
+else if (DMIB == DMIB_to) Kind = OMPC_MAP_to;
+else if (DMIB == DMIB_from) Kind = OMPC_MAP_from;

Use `switch`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16349
 
+static DefaultMapImplicitBehavior DefaultmapModifierToImplicitBehavior(
+  OpenMPDefaultmapClauseModifier M) {

1. Function names must start from verb.
2. Functions must start from lower letter.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16372
+
+static DefaultMapVariableCategory DefaultmapKindToVariableCategory(
+  OpenMPDefaultmapClauseKind Kind) {

1. Function names must start from verb.
2. Functions must start from lower letter.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16411-16420
+bool isDefaultmapModifier = (M == OMPC_DEFAULTMAP_MODIFIER_alloc) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_to) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_from) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_tofrom) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_firstprivate) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_default) ||
+(M == OMPC_DEFAULTMAP_MODIFIER_none);

Just `M != OMPC_DEFAULTMAP_MODIFIER_unknown` and `Kind != 
OMPC_DEFAULTMAP_unknown`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16426
+Value += "'scalar', 'aggregate', 'pointer'";
+Loc = KindLoc;
+Diag(Loc, diag::err_omp_unexpected_clause_value)

Why need to add a new `Loc` var here? Use the required `KindLoc`\`MLoc` 
directly.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > cchen wrote:
> > > > > ABataev wrote:
> > > > > > `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for 
> > > > > > others, it must be `llvm_unreachable()`
> > > > > 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`.
> > > > Then this function is not correct because for scalars default is not 
> > > > tofrom.
> > > 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).
> > You need to add extra checks for scalars and pointers in this function.
> 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.
Again, it means that this function is not correct in all cases and should be 
reworked somehow or deleted.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > cchen wrote:
> > > > ABataev wrote:
> > > > > `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for 
> > > > > others, it must be `llvm_unreachable()`
> > > > 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`.
> > > Then this function is not correct because for scalars default is not 
> > > tofrom.
> > 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).
> You need to add extra checks for scalars and pointers in this function.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

cchen wrote:
> ABataev wrote:
> > cchen wrote:
> > > ABataev wrote:
> > > > `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for others, 
> > > > it must be `llvm_unreachable()`
> > > 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`.
> > Then this function is not correct because for scalars default is not tofrom.
> 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).
You need to add extra checks for scalars and pointers in this function.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

ABataev wrote:
> cchen wrote:
> > ABataev wrote:
> > > `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for others, it 
> > > must be `llvm_unreachable()`
> > 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`.
> Then this function is not correct because for scalars default is not tofrom.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

cchen wrote:
> ABataev wrote:
> > `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for others, it 
> > must be `llvm_unreachable()`
> 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`.
Then this function is not correct because for scalars default is not tofrom.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1967
   (DSAStack->isForceCaptureByReferenceInTargetExecutable() &&
!Ty->isAnyPointerType()) ||
   !Ty->isScalarType() ||

ABataev wrote:
> ABataev wrote:
> > Seems to me, you're missing additional checks for pointers here.
> Not done
I think I check for `pointer` in line 1897-1898 or I could just misunderstand 
what you are saying.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

ABataev wrote:
> `OMPC_MAP_tofrom` must be returned only for `DMIB_tofrom`, for others, it 
> must be `llvm_unreachable()`
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`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1967
   (DSAStack->isForceCaptureByReferenceInTargetExecutable() &&
!Ty->isAnyPointerType()) ||
   !Ty->isScalarType() ||

ABataev wrote:
> Seems to me, you're missing additional checks for pointers here.
Not done



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2830
+return DMVC_pointer;
+  } else if (VD->getType()->isScalarType()) {
+return DMVC_scalar;

No need `else if` here , just `if`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2832
+return DMVC_scalar;
+  } else {
+return DMVC_aggregate;

No need for `else` here, just `return`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3200-3201
   }
-  ArrayRef getImplicitMap() const { return ImplicitMap; }
+  std::array, DMVC_unspecified> getImplicitMap()
+  const { return ImplicitMap; }
   const Sema::VarsWithInheritedDSAType () const {

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4374
+  case DMIB_unspecified:
+return OMPC_MAP_tofrom;
+  }

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:16512
+case OMPC_DEFAULTMAP_MODIFIER_default:
+  DSAStack->setDefaultDMAAttr(DMIB_default, DMVC_pointer, StartLoc);
+  break;

Just create 2 variables and initialize them. Call `setDefault...` only once.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7246-7248
+// Set implicit behavior except for "default" for defaultmap
+if ((Bits & OMP_MAP_IMPLICIT) &&
+(ImplicitBehavior != OMPC_DEFAULTMAP_MODIFIER_default)) {

cchen wrote:
> ABataev wrote:
> > 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.
> 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?
Just create 3 arrays instead of single array for mapped items and call 
`ActOnOpenMPMapClause` for each of them



Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+DefaultmapInfo DefaultmapMap[3];
+

cchen wrote:
> ABataev wrote:
> > ABataev wrote:
> > > 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?
> > Not done.
> 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
Use `DefaultmapInfo DefaultmapMap[DMVC_unspecified]` instead of `DefaultmapInfo 
DefaultmapMap[3]`, this is what I meant.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 2 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7246-7248
+// Set implicit behavior except for "default" for defaultmap
+if ((Bits & OMP_MAP_IMPLICIT) &&
+(ImplicitBehavior != OMPC_DEFAULTMAP_MODIFIER_default)) {

ABataev wrote:
> 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.
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?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+DefaultmapInfo DefaultmapMap[3];
+

ABataev wrote:
> ABataev wrote:
> > 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?
> Not done.
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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Some comments marked as ]Вщту] are not done actually, check them, please.




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7246-7248
+// Set implicit behavior except for "default" for defaultmap
+if ((Bits & OMP_MAP_IMPLICIT) &&
+(ImplicitBehavior != OMPC_DEFAULTMAP_MODIFIER_default)) {

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.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:8342
"Not expecting declaration with no component lists.");
-DeclComponentLists.emplace_back(L.second, C->getMapType(),
+DeclComponentLists.emplace_back(L.second, C->getMapType(),  // 
getMapType been hardcoded as tofrom
 C->getMapTypeModifiers(),

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+DefaultmapInfo DefaultmapMap[3];
+

ABataev wrote:
> 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?
Not done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7368-7370
+  bool IsImplicit, OpenMPDefaultmapClauseModifier ImplicitBehavior =
+  OMPC_DEFAULTMAP_MODIFIER_default,
+  OpenMPDefaultmapClauseKind VariableCategory = OMPC_DEFAULTMAP_unknown,

ABataev wrote:
> Do you really need the default params values here?
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 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.def:426-436
+OPENMP_DEFAULTMAP_KIND(aggregate)
+OPENMP_DEFAULTMAP_KIND(pointer)
 
 // Modifiers for 'defaultmap' clause.
+OPENMP_DEFAULTMAP_MODIFIER(alloc)
+OPENMP_DEFAULTMAP_MODIFIER(to)
+OPENMP_DEFAULTMAP_MODIFIER(from)

cchen wrote:
> ABataev wrote:
> > 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
> 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!
No, not in this file, this is just general comment. You need to guard this 
during semantic analysis


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.def:426-436
+OPENMP_DEFAULTMAP_KIND(aggregate)
+OPENMP_DEFAULTMAP_KIND(pointer)
 
 // Modifiers for 'defaultmap' clause.
+OPENMP_DEFAULTMAP_MODIFIER(alloc)
+OPENMP_DEFAULTMAP_MODIFIER(to)
+OPENMP_DEFAULTMAP_MODIFIER(from)

ABataev wrote:
> 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
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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.def:426-436
+OPENMP_DEFAULTMAP_KIND(aggregate)
+OPENMP_DEFAULTMAP_KIND(pointer)
 
 // Modifiers for 'defaultmap' clause.
+OPENMP_DEFAULTMAP_MODIFIER(alloc)
+OPENMP_DEFAULTMAP_MODIFIER(to)
+OPENMP_DEFAULTMAP_MODIFIER(from)

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



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7274
+  case OMPC_DEFAULTMAP_MODIFIER_none:
+Bits = OMP_MAP_NONE;
+break;

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



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7276
+break;
+  default:
+llvm_unreachable("Unexpected implicit behavior!");

It is not recommended to use `default:` case in switches over enumerics, just 
add a case for each enum value and remove `default:`



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7368-7370
+  bool IsImplicit, OpenMPDefaultmapClauseModifier ImplicitBehavior =
+  OMPC_DEFAULTMAP_MODIFIER_default,
+  OpenMPDefaultmapClauseKind VariableCategory = OMPC_DEFAULTMAP_unknown,

Do you really need the default params values here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:134
+DefaultMapVariableCategory VariableCategory = DMVC_unspecified;
+SourceLocation SLoc = SourceLocation();
+DefaultmapInfo() = default;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:149
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+DefaultmapInfo DefaultmapMap[3];
+

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?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:623
+auto  = getTopOfStack().
+   DefaultmapMap[static_cast(DMVC-1)];
+DefaultmapInfo.ImplicitBehavior = DMIB;

Do you really need the static_cast here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:625
+DefaultmapInfo.ImplicitBehavior = DMIB;
+DefaultmapInfo.VariableCategory = DMVC;
+DefaultmapInfo.SLoc = Loc;

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:630
+  bool hasSetDefaultmapCategory(OpenMPDefaultmapClauseKind VariableCategory) {
+int VC = static_cast(VariableCategory);
+return getTopOfStack().DefaultmapMap[VC].ImplicitBehavior != 
DMIB_unspecified &&

Do you really need a cast here?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:656
+ DefaultMapVariableCategory DMVC) const {
+if (DMVC == DMVC_scalar) {
+  return (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==

Better to use switch here.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:657-673
+  return (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+DMIB_alloc) ||
+ (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+DMIB_to) ||
+ (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+DMIB_from) ||
+ (getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2928
+bool IsDMIBNone = false;
+if (VD->getType()->isPointerType() || 
VD->getType()->isMemberPointerType()) {
+  IsDMIBNone =

Just `isAnyPointerType()`



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2932-2933
+} else if (VD->getType()->isScalarType() &&
+   !VD->getType()->isBlockPointerType() &&
+   !VD->getType()->isObjCObjectPointerType()) {
+  IsDMIBNone =

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2936
+Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) == DMIB_none;
+} else if (VD->getType()->isArrayType() || 
VD->getType()->isRecordType()) {
+  IsDMIBNone =

Just `isAggregateType()`? 



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2996-3003
+

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked 12 inline comments as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1978
+   DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_firstprivate) ||
   DSAStack->hasExplicitDSA(

ABataev wrote:
> If the scalars are mapped as firstprivates by default, they must be captured 
> by value.
I somehow forgot about this, will fix this soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Some of the previous comments are not fixed.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9100
+def note_omp_defaultmap_attr_none : Note<
+  "explicit data sharing attribute, data mapping attribute, or is_devise_ptr 
clause requested here">;
 def err_omp_wrong_dsa : Error<

is_device_ptr, not is_devise_ptr




Comment at: clang/lib/Sema/SemaOpenMP.cpp:1969-1972
+  (DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_alloc ||
+   DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_to ||

cchen wrote:
> ABataev wrote:
> > I think, alloc and to scalars also must be captured by value. Moreover, 
> > seems to me, alloc scalars should not be captured at all since their 
> > behavior is very similar to the behavior of private variables.
> Shouldn't defaultmap(alloc/to:scalar) have the same behavior (copy by ref) as 
> map(alloc/to) for scalar? Also, I don't think defaultmap(alloc:scalar) should 
> be the same as private (not copy at all) since you still need to register the 
> scalar address on the device.
Ah, yes, I forgot that we actually allocate variables on the host. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1969-1972
+  (DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_alloc ||
+   DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_to ||

ABataev wrote:
> I think, alloc and to scalars also must be captured by value. Moreover, seems 
> to me, alloc scalars should not be captured at all since their behavior is 
> very similar to the behavior of private variables.
Shouldn't defaultmap(alloc/to:scalar) have the same behavior (copy by ref) as 
map(alloc/to) for scalar? Also, I don't think defaultmap(alloc:scalar) should 
be the same as private (not copy at all) since you still need to register the 
scalar address on the device.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:1987-2001
+  /// Build a new OpenMP 'defaultmap' clause.
+  ///
+  /// By default, performs semantic analysis to build the new OpenMP clause.
+  /// Subclasses may override this routine to provide different behavior.
+  OMPClause *RebuildOMPDefaultmapClause(OpenMPDefaultmapClauseModifier M,
+OpenMPDefaultmapClauseKind Kind,
+SourceLocation StartLoc,

cchen wrote:
> ABataev wrote:
> > Do you really need to rebuild it? It has only constants inside.
> I think you are right, the rebuild code for defaultmap seems redundant since 
> it has only constant inside the function. I added this code so that my 
> defaultmap(none) check inside DSAAttrChecker could be called for the second 
> time (after template initialization). Without adding this code, the check in 
> DSAAttrChecker for defaultmap(none) will never be called due to the guard in 
> VisitDeclRefExpr:
> ```
> if (E->isTypeDependent() || E->isValueDependent() ||
> E->containsUnexpandedParameterPack() || E->isInstantiationDependent())
> ```
> Where do you think I should put my check for defaultmap(none) so that I don't 
> need this redundant tree rebuild code? Thanks!
Ok, got it. Then better to rebuild the clause. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/TreeTransform.h:1987-2001
+  /// Build a new OpenMP 'defaultmap' clause.
+  ///
+  /// By default, performs semantic analysis to build the new OpenMP clause.
+  /// Subclasses may override this routine to provide different behavior.
+  OMPClause *RebuildOMPDefaultmapClause(OpenMPDefaultmapClauseModifier M,
+OpenMPDefaultmapClauseKind Kind,
+SourceLocation StartLoc,

ABataev wrote:
> Do you really need to rebuild it? It has only constants inside.
I think you are right, the rebuild code for defaultmap seems redundant since it 
has only constant inside the function. I added this code so that my 
defaultmap(none) check inside DSAAttrChecker could be called for the second 
time (after template initialization). Without adding this code, the check in 
DSAAttrChecker for defaultmap(none) will never be called due to the guard in 
VisitDeclRefExpr:
```
if (E->isTypeDependent() || E->isValueDependent() ||
E->containsUnexpandedParameterPack() || E->isInstantiationDependent())
```
Where do you think I should put my check for defaultmap(none) so that I don't 
need this redundant tree rebuild code? Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:137-140
+ SourceLocation Loc) {
+  ImplicitBehavior = IB;
+  VariableCategory = VC;
+  SLoc = Loc;

Use member initializers in constructors.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:152
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+std::array DefaultmapMap;
+

No, I meant just a C array, like `DefaultmapInfo DefaultMap[3];`.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:620-624
+  /// Set default data mapping attribute to 'alloc:scalar'.
+  void setDefaultDMAAllocScalar(SourceLocation Loc) {
+getTopOfStack().DefaultmapMap[OMPC_DEFAULTMAP_scalar]
+  .set(DMIB_alloc, DMVC_scalar, Loc);
+  }

Why do we need so many functions? Better to have just one function with 
additional params.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1967
   (DSAStack->isForceCaptureByReferenceInTargetExecutable() &&
!Ty->isAnyPointerType()) ||
   !Ty->isScalarType() ||

Seems to me, you're missing additional checks for pointers here.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1969-1972
+  (DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_alloc ||
+   DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_to ||

I think, alloc and to scalars also must be captured by value. Moreover, seems 
to me, alloc scalars should not be captured at all since their behavior is very 
similar to the behavior of private variables.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:1978
+   DSAStack->getDefaultDMIBAtLevel(Level, OMPC_DEFAULTMAP_scalar) ==
+   DMIB_firstprivate) ||
   DSAStack->hasExplicitDSA(

If the scalars are mapped as firstprivates by default, they must be captured by 
value.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2200-2201
+  DMIB_tofrom &&
+  DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_scalar) !=
+  DMIB_firstprivate) &&
+  !(D->getType()->isPointerType() && // TODO check

Seems to me, this is wrong. If the default is firstprivate the variables must 
be marked as firstprivate. Maybe, just check if the default for scalars is not 
specified or is firstprivate instead of so many checks?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2202-2212
+  !(D->getType()->isPointerType() && // TODO check
+  (DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) 
==
+  DMIB_alloc ||
+  DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+  DMIB_to ||
+  DSAStack->getDefaultDMIBAtLevel(NewLevel, OMPC_DEFAULTMAP_pointer) ==
+  DMIB_from ||

1. Also, check the logic here. 
2. Why there is TODO?



Comment at: clang/lib/Sema/SemaOpenMP.cpp:3059-3082
+  (((VD->getType().getNonReferenceType()->isScalarType() &&
+   Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_alloc &&
+   Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_to &&
+   Stack->getDefaultDMIB(OMPC_DEFAULTMAP_scalar) !=
+ DMIB_from &&

I see similar cehcks in different places, Outline them into a standalone 
function.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4466-4472
+
   if (AStmt && !CurContext->isDependentContext()) {
 assert(isa(AStmt) && "Captured statement expected");
 
 // Check default data sharing attributes for referenced variables.
 DSAAttrChecker DSAChecker(DSAStack, *this, cast(AStmt));
+

Extra blank lines



Comment at: clang/lib/Sema/TreeTransform.h:1987-2001
+  /// Build a new OpenMP 'defaultmap' clause.
+  ///
+  /// By default, performs semantic analysis to build the new OpenMP clause.
+  /// Subclasses may override this routine to provide different behavior.
+  OMPClause *RebuildOMPDefaultmapClause(OpenMPDefaultmapClauseModifier M,
+OpenMPDefaultmapClauseKind Kind,
+SourceLocation StartLoc,

Do you really need to rebuild it? It has only constants inside.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4463-4890
+
   if (AStmt && !CurContext->isDependentContext()) {
 assert(isa(AStmt) && "Captured statement expected");
 
 // Check default data sharing attributes for referenced variables.
 DSAAttrChecker DSAChecker(DSAStack, *this, cast(AStmt));
+

cchen wrote:
> cchen wrote:
> > ABataev wrote:
> > > The formatting changes better to put in a separate NFC patch if you think 
> > > that this is required.
> > I don't really understand why adding a specific diagnostic message for 
> > defaultmap(none: x) is just a formatting change.  For defaultmap(none: x), 
> > we emit error message if there are no explicity specified data-sharing 
> > attributes, data mapping attributes, or is_device_ptr clause, while for 
> > default(none), we emit error message if there are no explicitly specified 
> > data-sharing attributes. So the message for defaultmap(none: x) should be 
> > different from default(none).
> Oh, my bad, you mean this new line. I'll delete it. Thanks
Yes, I was talking about new empty lines. Do not add them here and other places.



Comment at: 
clang/test/OpenMP/target_teams_distribute_simd_defaultmap_messages.cpp:55
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd defaultmap (tofrom, scalar // 
expected-error {{expected ')'}} expected-warning {{missing ':' after defaultmap 
modifier - ignoring}} expected-error {{expected 'scalar' in OpenMP clause 
'defaultmap'}} expected-note {{to match this '('}}
+#pragma omp target teams distribute simd defaultmap (tofrom, scalar // 
expected-error {{expected ')'}} expected-warning {{missing ':' after defaultmap 
modifier - ignoring}} expected-error {{expected ['scalar', 'aggregate', 
'pointer'] in OpenMP clause 'defaultmap'}} expected-note {{to match this '('}}
   for (i = 0; i < argc; ++i) foo();

We never used `[]` in messages, please remove them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen marked an inline comment as done.
cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4463-4890
+
   if (AStmt && !CurContext->isDependentContext()) {
 assert(isa(AStmt) && "Captured statement expected");
 
 // Check default data sharing attributes for referenced variables.
 DSAAttrChecker DSAChecker(DSAStack, *this, cast(AStmt));
+

cchen wrote:
> ABataev wrote:
> > The formatting changes better to put in a separate NFC patch if you think 
> > that this is required.
> I don't really understand why adding a specific diagnostic message for 
> defaultmap(none: x) is just a formatting change.  For defaultmap(none: x), we 
> emit error message if there are no explicity specified data-sharing 
> attributes, data mapping attributes, or is_device_ptr clause, while for 
> default(none), we emit error message if there are no explicitly specified 
> data-sharing attributes. So the message for defaultmap(none: x) should be 
> different from default(none).
Oh, my bad, you mean this new line. I'll delete it. Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4463-4890
+
   if (AStmt && !CurContext->isDependentContext()) {
 assert(isa(AStmt) && "Captured statement expected");
 
 // Check default data sharing attributes for referenced variables.
 DSAAttrChecker DSAChecker(DSAStack, *this, cast(AStmt));
+

ABataev wrote:
> The formatting changes better to put in a separate NFC patch if you think 
> that this is required.
I don't really understand why adding a specific diagnostic message for 
defaultmap(none: x) is just a formatting change.  For defaultmap(none: x), we 
emit error message if there are no explicity specified data-sharing attributes, 
data mapping attributes, or is_device_ptr clause, while for default(none), we 
emit error message if there are no explicitly specified data-sharing 
attributes. So the message for defaultmap(none: x) should be different from 
default(none).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69204: [OpenMP 5.0] - Extend defaultmap

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:52-74
 enum DefaultMapAttributes {
-  DMA_unspecified,   /// Default mapping is not specified.
-  DMA_tofrom_scalar, /// Default mapping is 'tofrom:scalar'.
+  DMA_unspecified, /// Default mapping is not specified.
+  DMA_alloc_scalar,/// Default mapping is 'alloc:scalar'.
+  DMA_to_scalar,   /// Default mapping is 'to:scalar'.
+  DMA_from_scalar, /// Default mapping is 'from:scalar'.
+  DMA_tofrom_scalar,   /// Default mapping is 'tofrom:scalar'.
+  DMA_firstprivate_scalar, /// Default mapping is 'firstprivate:scalar'.

Bad decision, better to add 2 new enums, one for behavior and one for the 
variable category.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:147-148
 SourceLocation DefaultAttrLoc;
-DefaultMapAttributes DefaultMapAttr = DMA_unspecified;
-SourceLocation DefaultMapAttrLoc;
+llvm::SmallVector, 3>
+  DefaultmapMap{3, std::make_pair(DMA_unspecified, SourceLocation())};
 OpenMPDirectiveKind Directive = OMPD_unknown;

No need to use the vector here, use the regular array



Comment at: clang/lib/Sema/SemaOpenMP.cpp:2983-2992
+  // OpenMP 5.0 [2.19.7.2, defaultmap clause, Description]
+  // If implicit-behavior is none, 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-haring attribute clause (including a data-sharing attribute
+  // clause on a combined construct where target. is one of the

It must be active only for OpenMP >= 50.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4463-4890
+
   if (AStmt && !CurContext->isDependentContext()) {
 assert(isa(AStmt) && "Captured statement expected");
 
 // Check default data sharing attributes for referenced variables.
 DSAAttrChecker DSAChecker(DSAStack, *this, cast(AStmt));
+

The formatting changes better to put in a separate NFC patch if you think that 
this is required.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4899-4903
+} else {
+  Diag(P.second->getExprLoc(), 
diag::err_omp_defaultmap_no_attr_for_variable)
+  << P.first << P.second->getSourceRange();
+  Diag(DSAStack->getDefaultDSALocation(), 
diag::note_omp_defaultmap_attr_none);
+}

Add a check for OpenMP version.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69204/new/

https://reviews.llvm.org/D69204



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits