[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Thanks.  It probably won't be soon.  At the very least, I need to find the 
answer to question 3 from the review summary.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83061#2165100 , @jdenny wrote:

> In D83061#2165093 , @ABataev wrote:
>
> > In D83061#2165089 , @jdenny wrote:
> >
> > > In D83061#2165063 , @ABataev 
> > > wrote:
> > >
> > > > LG.
> > >
> > >
> > > Thanks for the review.
> > >
> > > As discussed in the review summary, please consider the following.  A 
> > > present map type modifier behavior that this patch does not attempt to 
> > > implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:
> > >
> > > > If a map clause with a present map-type-modifier is present in a map
> > > >  clause, then the effect of the clause is ordered before all other
> > > >  map clauses that do not have the present modifier.
> > >
> > > Compare to L10-11:
> > >
> > > > For a given construct, the effect of a map clause with the to, from,
> > > >  or tofrom map-type is ordered before the effect of a map clause with
> > > >  the alloc, release, or delete map-type.
> > >
> > > As far as I can tell, Clang does not implement L10-11. Is that correct?  
> > > If not, then I think both passages should be implemented together later.  
> > > Any objections?
> >
> >
> > Looks like you're right. Yes, go ahead and implement it.
>
>
> Are you ok for it to be a later patch after pushing these?


Sure


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D83061#2165093 , @ABataev wrote:

> In D83061#2165089 , @jdenny wrote:
>
> > In D83061#2165063 , @ABataev wrote:
> >
> > > LG.
> >
> >
> > Thanks for the review.
> >
> > As discussed in the review summary, please consider the following.  A 
> > present map type modifier behavior that this patch does not attempt to 
> > implement is TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:
> >
> > > If a map clause with a present map-type-modifier is present in a map
> > >  clause, then the effect of the clause is ordered before all other
> > >  map clauses that do not have the present modifier.
> >
> > Compare to L10-11:
> >
> > > For a given construct, the effect of a map clause with the to, from,
> > >  or tofrom map-type is ordered before the effect of a map clause with
> > >  the alloc, release, or delete map-type.
> >
> > As far as I can tell, Clang does not implement L10-11. Is that correct?  If 
> > not, then I think both passages should be implemented together later.  Any 
> > objections?
>
>
> Looks like you're right. Yes, go ahead and implement it.


Are you ok for it to be a later patch after pushing these?


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D83061#2165089 , @jdenny wrote:

> In D83061#2165063 , @ABataev wrote:
>
> > LG.
>
>
> Thanks for the review.
>
> As discussed in the review summary, please consider the following.  A present 
> map type modifier behavior that this patch does not attempt to implement is 
> TR8 sec. 2.22.7.1 "map Clause", p. 319, L14-16:
>
> > If a map clause with a present map-type-modifier is present in a map
> >  clause, then the effect of the clause is ordered before all other
> >  map clauses that do not have the present modifier.
>
> Compare to L10-11:
>
> > For a given construct, the effect of a map clause with the to, from,
> >  or tofrom map-type is ordered before the effect of a map clause with
> >  the alloc, release, or delete map-type.
>
> As far as I can tell, Clang does not implement L10-11. Is that correct?  If 
> not, then I think both passages should be implemented together later.  Any 
> objections?


Looks like you're right. Yes, go ahead and implement it.

> 
> 
>> Please, land it after the runtime part
> 
> I'll push them at the same time.  The runtime patch needs to be second 
> because its test suite depends on this patch.  But there are still some 
> details to resolve in the runtime patch as well.




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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

In D83061#2165063 , @ABataev wrote:

> LG.


Thanks for the review.

As discussed in the review summary, please consider the following.  A present 
map type modifier behavior that this patch does not attempt to implement is TR8 
sec. 2.22.7.1 "map Clause", p. 319, L14-16:

>   If a map clause with a present map-type-modifier is present in a map
>   clause, then the effect of the clause is ordered before all other
>   map clauses that do not have the present modifier.

Compare to L10-11:

> For a given construct, the effect of a map clause with the to, from,
>  or tofrom map-type is ordered before the effect of a map clause with
>  the alloc, release, or delete map-type.

As far as I can tell, Clang does not implement L10-11. Is that correct?  If 
not, then I think both passages should be implemented together later.  Any 
objections?

> Please, land it after the runtime part

I'll push them at the same time.  The runtime patch needs to be second because 
its test suite depends on this patch.  But there are still some details to 
resolve in the runtime patch as well.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[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. Please, land it after the runtime part


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > Why do we need this?
> > > > > > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return 
> > > > > > either an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` 
> > > > > > depending on `Tok`.  Thus, without this change, both 
> > > > > > `isMapModifier` and `isMapType` can return either of those despite 
> > > > > > the difference in their names and documentation.
> > > > > > 
> > > > > > I found that, when `Parser::parseMapTypeModifiers` ignores 
> > > > > > `present` as if it's not a modifier because OpenMP < 5.1, then 
> > > > > > `isMapType` later returns `OMPC_MAP_MODIFIER_present` and causes 
> > > > > > the following assert to fail in `Sema::ActOnOpenMPVarListClause`:
> > > > > > 
> > > > > > ```
> > > > > > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
> > > > > >"Unexpected map modifier.");
> > > > > > ```
> > > > > > 
> > > > > > To me, the most obvious solution is to fix `isMapType` and 
> > > > > > `isMapModifier`.  Fortunately, looking in `OpenMPKinds.h`, the 
> > > > > > enumerator values in `OpenMPMapModifierKind` and 
> > > > > > `OpenMPMapClauseKind` are disjoint so we can tell which we have.
> > > > > Can we have something like:
> > > > > ```
> > > > > if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
> > > > >   return OMPC_MAP_MODIFIER_unknown;
> > > > > ```
> > > > > or extend `getOpenMPSimpleClauseType` function with the version 
> > > > > parameter and check there is modifier is allowed and return `unknown` 
> > > > > if it is not compatible with provided version?
> > > > As far as I can tell, my changes general fix this bug in `isMapType` 
> > > > and `isMapModifier`.  It makes them behave as documented.  The 
> > > > suggestions you're making only fix them for the case of `present`.  Why 
> > > > is that better?
> > > It is too general. I think it may mask some issues in future. That's why 
> > > I would suggest to tweak it for `present` only. Or, even better, extend 
> > > `getOpenMPSimpleClauseType` function to handle this modifiers more 
> > > correctly for each particular version rather than fix it here.
> > > Or, even better, extend getOpenMPSimpleClauseType function to handle this 
> > > modifiers more correctly for each particular version rather than fix it 
> > > here.
> > 
> > One way to implement what I think you mean is to add an additional 
> > parameter to `getOpenMPSimpleClauseType` to request either the clause's 
> > associated type or that type's modifiers.  Unless I missed a clause, this 
> > parameter would affect only map, defaultmap, and schedule clauses.  For 
> > other clauses, this parameter would be ignored.  Is that what you have in 
> > mind?
> I would also add a check for version compatibility if the modifier is 
> supported for the given version. But anyway, I would like to take a look at 
> what you have in mind
I think I misread your previous comment.  I believe I've implemented your 
suggestion now.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > ABataev wrote:
> > > > > > Why do we need this?
> > > > > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return 
> > > > > either an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending 
> > > > > on `Tok`.  Thus, without this change, both `isMapModifier` and 
> > > > > `isMapType` can return either of those despite the difference in 
> > > > > their names and documentation.
> > > > > 
> > > > > I found that, when `Parser::parseMapTypeModifiers` ignores `present` 
> > > > > as if it's not a modifier because OpenMP < 5.1, then `isMapType` 
> > > > > later returns `OMPC_MAP_MODIFIER_present` and causes the following 
> > > > > assert to fail in `Sema::ActOnOpenMPVarListClause`:
> > > > > 
> > > > > ```
> > > > > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
> > > > >"Unexpected map modifier.");
> > > > > ```
> > > > > 
> > > > > To me, the most obvious solution is to fix `isMapType` and 
> > > > > `isMapModifier`.  Fortunately, looking in `OpenMPKinds.h`, the 
> > > > > enumerator values in `OpenMPMapModifierKind` and 
> > > > > `OpenMPMapClauseKind` are disjoint so we can tell which we have.
> > > > Can we have something like:
> > > > ```
> > > > if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
> > > >   return OMPC_MAP_MODIFIER_unknown;
> > > > ```
> > > > or extend `getOpenMPSimpleClauseType` function with the version 
> > > > parameter and check there is modifier is allowed and return `unknown` 
> > > > if it is not compatible with provided version?
> > > As far as I can tell, my changes general fix this bug in `isMapType` and 
> > > `isMapModifier`.  It makes them behave as documented.  The suggestions 
> > > you're making only fix them for the case of `present`.  Why is that 
> > > better?
> > It is too general. I think it may mask some issues in future. That's why I 
> > would suggest to tweak it for `present` only. Or, even better, extend 
> > `getOpenMPSimpleClauseType` function to handle this modifiers more 
> > correctly for each particular version rather than fix it here.
> > Or, even better, extend getOpenMPSimpleClauseType function to handle this 
> > modifiers more correctly for each particular version rather than fix it 
> > here.
> 
> One way to implement what I think you mean is to add an additional parameter 
> to `getOpenMPSimpleClauseType` to request either the clause's associated type 
> or that type's modifiers.  Unless I missed a clause, this parameter would 
> affect only map, defaultmap, and schedule clauses.  For other clauses, this 
> parameter would be ignored.  Is that what you have in mind?
I would also add a check for version compatibility if the modifier is supported 
for the given version. But anyway, I would like to take a look at what you have 
in mind


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

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



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > Why do we need this?
> > > > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return 
> > > > either an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on 
> > > > `Tok`.  Thus, without this change, both `isMapModifier` and `isMapType` 
> > > > can return either of those despite the difference in their names and 
> > > > documentation.
> > > > 
> > > > I found that, when `Parser::parseMapTypeModifiers` ignores `present` as 
> > > > if it's not a modifier because OpenMP < 5.1, then `isMapType` later 
> > > > returns `OMPC_MAP_MODIFIER_present` and causes the following assert to 
> > > > fail in `Sema::ActOnOpenMPVarListClause`:
> > > > 
> > > > ```
> > > > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
> > > >"Unexpected map modifier.");
> > > > ```
> > > > 
> > > > To me, the most obvious solution is to fix `isMapType` and 
> > > > `isMapModifier`.  Fortunately, looking in `OpenMPKinds.h`, the 
> > > > enumerator values in `OpenMPMapModifierKind` and `OpenMPMapClauseKind` 
> > > > are disjoint so we can tell which we have.
> > > Can we have something like:
> > > ```
> > > if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
> > >   return OMPC_MAP_MODIFIER_unknown;
> > > ```
> > > or extend `getOpenMPSimpleClauseType` function with the version parameter 
> > > and check there is modifier is allowed and return `unknown` if it is not 
> > > compatible with provided version?
> > As far as I can tell, my changes general fix this bug in `isMapType` and 
> > `isMapModifier`.  It makes them behave as documented.  The suggestions 
> > you're making only fix them for the case of `present`.  Why is that better?
> It is too general. I think it may mask some issues in future. That's why I 
> would suggest to tweak it for `present` only. Or, even better, extend 
> `getOpenMPSimpleClauseType` function to handle this modifiers more correctly 
> for each particular version rather than fix it here.
> Or, even better, extend getOpenMPSimpleClauseType function to handle this 
> modifiers more correctly for each particular version rather than fix it here.

One way to implement what I think you mean is to add an additional parameter to 
`getOpenMPSimpleClauseType` to request either the clause's associated type or 
that type's modifiers.  Unless I missed a clause, this parameter would affect 
only map, defaultmap, and schedule clauses.  For other clauses, this parameter 
would be ignored.  Is that what you have in mind?


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > Why do we need this?
> > > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return 
> > > either an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on 
> > > `Tok`.  Thus, without this change, both `isMapModifier` and `isMapType` 
> > > can return either of those despite the difference in their names and 
> > > documentation.
> > > 
> > > I found that, when `Parser::parseMapTypeModifiers` ignores `present` as 
> > > if it's not a modifier because OpenMP < 5.1, then `isMapType` later 
> > > returns `OMPC_MAP_MODIFIER_present` and causes the following assert to 
> > > fail in `Sema::ActOnOpenMPVarListClause`:
> > > 
> > > ```
> > > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
> > >"Unexpected map modifier.");
> > > ```
> > > 
> > > To me, the most obvious solution is to fix `isMapType` and 
> > > `isMapModifier`.  Fortunately, looking in `OpenMPKinds.h`, the enumerator 
> > > values in `OpenMPMapModifierKind` and `OpenMPMapClauseKind` are disjoint 
> > > so we can tell which we have.
> > Can we have something like:
> > ```
> > if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
> >   return OMPC_MAP_MODIFIER_unknown;
> > ```
> > or extend `getOpenMPSimpleClauseType` function with the version parameter 
> > and check there is modifier is allowed and return `unknown` if it is not 
> > compatible with provided version?
> As far as I can tell, my changes general fix this bug in `isMapType` and 
> `isMapModifier`.  It makes them behave as documented.  The suggestions you're 
> making only fix them for the case of `present`.  Why is that better?
It is too general. I think it may mask some issues in future. That's why I 
would suggest to tweak it for `present` only. Or, even better, extend 
`getOpenMPSimpleClauseType` function to handle this modifiers more correctly 
for each particular version rather than fix it here.



Comment at: clang/test/OpenMP/target_data_codegen.cpp:258-260
+// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]

jdenny wrote:
> ABataev wrote:
> > Add a comment with interpretaion of the map flags, like `OMP_TO = 0x1 | 
> > OMP_FROM=0x2 | OMP_PRESENT = 0x1000 = 0x1003`
> Are you asking me to add that comment to every map type encoding appearing in 
> this patch?  Or just this one?
> 
It would be good to have something like this in all codegen tests you added in 
this patch


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

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



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > Why do we need this?
> > When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return either 
> > an `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on `Tok`.  
> > Thus, without this change, both `isMapModifier` and `isMapType` can return 
> > either of those despite the difference in their names and documentation.
> > 
> > I found that, when `Parser::parseMapTypeModifiers` ignores `present` as if 
> > it's not a modifier because OpenMP < 5.1, then `isMapType` later returns 
> > `OMPC_MAP_MODIFIER_present` and causes the following assert to fail in 
> > `Sema::ActOnOpenMPVarListClause`:
> > 
> > ```
> > assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
> >"Unexpected map modifier.");
> > ```
> > 
> > To me, the most obvious solution is to fix `isMapType` and `isMapModifier`. 
> >  Fortunately, looking in `OpenMPKinds.h`, the enumerator values in 
> > `OpenMPMapModifierKind` and `OpenMPMapClauseKind` are disjoint so we can 
> > tell which we have.
> Can we have something like:
> ```
> if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
>   return OMPC_MAP_MODIFIER_unknown;
> ```
> or extend `getOpenMPSimpleClauseType` function with the version parameter and 
> check there is modifier is allowed and return `unknown` if it is not 
> compatible with provided version?
As far as I can tell, my changes general fix this bug in `isMapType` and 
`isMapModifier`.  It makes them behave as documented.  The suggestions you're 
making only fix them for the case of `present`.  Why is that better?



Comment at: clang/test/OpenMP/target_data_codegen.cpp:258-260
+// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]

ABataev wrote:
> Add a comment with interpretaion of the map flags, like `OMP_TO = 0x1 | 
> OMP_FROM=0x2 | OMP_PRESENT = 0x1000 = 0x1003`
Are you asking me to add that comment to every map type encoding appearing in 
this patch?  Or just this one?



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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

jdenny wrote:
> ABataev wrote:
> > Why do we need this?
> When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return either an 
> `OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on `Tok`.  Thus, 
> without this change, both `isMapModifier` and `isMapType` can return either 
> of those despite the difference in their names and documentation.
> 
> I found that, when `Parser::parseMapTypeModifiers` ignores `present` as if 
> it's not a modifier because OpenMP < 5.1, then `isMapType` later returns 
> `OMPC_MAP_MODIFIER_present` and causes the following assert to fail in 
> `Sema::ActOnOpenMPVarListClause`:
> 
> ```
> assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
>"Unexpected map modifier.");
> ```
> 
> To me, the most obvious solution is to fix `isMapType` and `isMapModifier`.  
> Fortunately, looking in `OpenMPKinds.h`, the enumerator values in 
> `OpenMPMapModifierKind` and `OpenMPMapClauseKind` are disjoint so we can tell 
> which we have.
Can we have something like:
```
if (LangOpts.OpenMP <= 50 && Type == OMPC_MAP_MODIFIER_present)
  return OMPC_MAP_MODIFIER_unknown;
```
or extend `getOpenMPSimpleClauseType` function with the version parameter and 
check there is modifier is allowed and return `unknown` if it is not compatible 
with provided version?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3103
 /// map([ [map-type-modifier[,] [map-type-modifier[,] ...] map-type : ] list)
 /// where, map-type-modifier ::= always | close | mapper(mapper-identifier)
 bool Parser::parseMapTypeModifiers(OpenMPVarListDataTy ) {

Update a comment here



Comment at: clang/test/OpenMP/target_data_codegen.cpp:258-260
+// CK1A: [[MTYPE00:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1021]]]
+
+// CK1A: [[MTYPE01:@.+]] = {{.+}}constant [1 x i64] [i64 [[#0x1425]]]

Add a comment with interpretaion of the map flags, like `OMP_TO = 0x1 | 
OMP_FROM=0x2 | OMP_PRESENT = 0x1000 = 0x1003`


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7934-7940
+// If any element has the present modifier, then make sure the runtime
+// doesn't attempt to allocate the struct.
+if (CurTypes.end() !=
+llvm::find_if(CurTypes, [](OpenMPOffloadMappingFlags Type) {
+  return Type & OMP_MAP_PRESENT;
+}))
+  Types.back() |= OMP_MAP_PRESENT;

ABataev wrote:
> Why do we need this extra stuff here?
For example:

```
#pragma omp target map(present, tofrom: s.x[0:3])
```

This generates 2 map entries:

  - `s`: 0x1020
  - `s.x`: 0x11003

Without the above change, the 0x1000 is missing from the first entry.  As a 
result, `s` is allocated, and then the presence check for `s.x` incorrectly 
passes at run time.



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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny marked 4 inline comments as done.
jdenny added inline comments.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

ABataev wrote:
> Why do we need this?
When called with `OMPC_map`, `getOpenMPSimpleClauseType` can return either an 
`OpenMPMapModifierKind` or `OpenMPMapClauseKind` depending on `Tok`.  Thus, 
without this change, both `isMapModifier` and `isMapType` can return either of 
those despite the difference in their names and documentation.

I found that, when `Parser::parseMapTypeModifiers` ignores `present` as if it's 
not a modifier because OpenMP < 5.1, then `isMapType` later returns 
`OMPC_MAP_MODIFIER_present` and causes the following assert to fail in 
`Sema::ActOnOpenMPVarListClause`:

```
assert(0 <= ExtraModifier && ExtraModifier <= OMPC_MAP_unknown &&
   "Unexpected map modifier.");
```

To me, the most obvious solution is to fix `isMapType` and `isMapModifier`.  
Fortunately, looking in `OpenMPKinds.h`, the enumerator values in 
`OpenMPMapModifierKind` and `OpenMPMapClauseKind` are disjoint so we can tell 
which we have.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3150-3151
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type > OMPC_MAP_unknown)
+return OMPC_MAP_unknown;
+  return static_cast(Type);

ABataev wrote:
> Same, why do we need this?
Responded on `isMapModifier`.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7043-7044
 OMP_MAP_CLOSE = 0x400,
+/// Produce a runtime error if the data is not already allocated.
+OMP_MAP_PRESENT = 0x800,
 /// The 16 MSBs of the flags indicate whether the entry is member of some

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > Better to use thу next value to avoid compatibility issues with XLC. 
> > You mean 0x1000?
> Yes
Could you add a comment that 0x800 is currently reserved for compatibility, 
please?



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7934-7940
+// If any element has the present modifier, then make sure the runtime
+// doesn't attempt to allocate the struct.
+if (CurTypes.end() !=
+llvm::find_if(CurTypes, [](OpenMPOffloadMappingFlags Type) {
+  return Type & OMP_MAP_PRESENT;
+}))
+  Types.back() |= OMP_MAP_PRESENT;

Why do we need this extra stuff here?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3067-3068
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type < OMPC_MAP_MODIFIER_unknown)
+return OMPC_MAP_MODIFIER_unknown;
+  return static_cast(Type);

Why do we need this?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3133
+  Diag(Tok, diag::err_omp_unknown_map_type_modifier)
+  << (getLangOpts().OpenMP >= 51);
   ConsumeToken();

Better to use `(getLangOpts().OpenMP >= 51 ? 1 : 0)`, the `select` is of 
integer type.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:3150-3151
+  unsigned Type = getOpenMPSimpleClauseType(OMPC_map, PP.getSpelling(Tok));
+  if (Type > OMPC_MAP_unknown)
+return OMPC_MAP_unknown;
+  return static_cast(Type);

Same, why do we need this?


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1252-1253
+  "incorrect map type modifier, expected 'always', 'close', 'mapper', or 
'present'">;
+def err_omp_map_type_modifier_wrong_version : Error<
+  "map type modifier '%0' requires OpenMP version %1 or above">;
 def err_omp_map_type_missing : Error<

jdenny wrote:
> ABataev wrote:
> > Better to keep original message for <= 5.0. This is what we usually do
> What message do you mean?  Which `err_` diag id?
For <= 5.0 the error message should be the same.
```
def err_omp_unknown_map_type_modifier : Error<
  "incorrect map type modifier, expected 'always', 'close', %select{or 
'mapper'|'mapper', or 'presence'}0">;
```
In code:
```
Diag(..., diags::err_omp_unknown_map_type_modifier) << (LangOpts.OpenMP <= 50 ? 
0 : 1);
```


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1252-1253
+  "incorrect map type modifier, expected 'always', 'close', 'mapper', or 
'present'">;
+def err_omp_map_type_modifier_wrong_version : Error<
+  "map type modifier '%0' requires OpenMP version %1 or above">;
 def err_omp_map_type_missing : Error<

ABataev wrote:
> Better to keep original message for <= 5.0. This is what we usually do
What message do you mean?  Which `err_` diag id?


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1252-1253
+  "incorrect map type modifier, expected 'always', 'close', 'mapper', or 
'present'">;
+def err_omp_map_type_modifier_wrong_version : Error<
+  "map type modifier '%0' requires OpenMP version %1 or above">;
 def err_omp_map_type_missing : Error<

Better to keep original message for <= 5.0. This is what we usually do



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7043-7044
 OMP_MAP_CLOSE = 0x400,
+/// Produce a runtime error if the data is not already allocated.
+OMP_MAP_PRESENT = 0x800,
 /// The 16 MSBs of the flags indicate whether the entry is member of some

jdenny wrote:
> ABataev wrote:
> > Better to use thу next value to avoid compatibility issues with XLC. 
> You mean 0x1000?
Yes


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 278658.
jdenny added a comment.

Rebased onto latest D83922 .

Implemented rejection of `present` if not OpenMP >= 5.1.

Cleaned up tests some.


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

https://reviews.llvm.org/D83061

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_data_ast_print.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_parallel_map_messages.cpp
  clang/test/OpenMP/target_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -468,7 +468,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
@@ -543,7 +543,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
Index: clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
+++ clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
@@ -175,7 +175,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
@@ -287,7 +287,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
Index: clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
+++ 

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

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



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7043-7044
 OMP_MAP_CLOSE = 0x400,
+/// Produce a runtime error if the data is not already allocated.
+OMP_MAP_PRESENT = 0x800,
 /// The 16 MSBs of the flags indicate whether the entry is member of some

ABataev wrote:
> Better to use thу next value to avoid compatibility issues with XLC. 
You mean 0x1000?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

This new modifier must be enabed only for OpenMP 5.1, need to add the checks 
for OpenMP version. Also, this change should not affect the functionality of 
previous versions




Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:7043-7044
 OMP_MAP_CLOSE = 0x400,
+/// Produce a runtime error if the data is not already allocated.
+OMP_MAP_PRESENT = 0x800,
 /// The 16 MSBs of the flags indicate whether the entry is member of some

Better to use thу next value to avoid compatibility issues with XLC. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 278343.
jdenny edited the summary of this revision.
jdenny set the repository for this revision to rG LLVM Github Monorepo.
jdenny added a comment.

Rebased, and extracted D83922  as discussed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83061

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_data_ast_print.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_parallel_map_messages.cpp
  clang/test/OpenMP/target_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -468,7 +468,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
@@ -543,7 +543,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
Index: clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
+++ clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
@@ -175,7 +175,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
@@ -287,7 +287,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
Index: clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > ABataev wrote:
> > > > jdenny wrote:
> > > > > In today's OpenMP/LLVM call, it was pointed out that this issue 
> > > > > represents a general Clang bug for map clauses: map clauses generally 
> > > > > shouldn't be ignored just because their variables aren't referenced 
> > > > > in the construct.
> > > > > 
> > > > > Here, I tried to handle this issue only in the case of a `present` 
> > > > > modifier because I previously didn't consider whether the existing 
> > > > > behavior for other map types was a bug.  My solution here was similar 
> > > > > to what's done for `omp target data`, which doesn't have captures at 
> > > > > all.  Perhaps, this should generalized to other map types.
> > > > > 
> > > > > Another possibility might be to expand what's captured in Sema.
> > > > It must be fixed in a separate patch. We definitely should not modify 
> > > > sema here, just the codegen. Currently, it iterates through all 
> > > > captures in region, also need to iterate through the explicit maps.
> > > Thanks for the response.  I'd be happy to generalize and extract into a 
> > > new patch.
> > > 
> > > It looks like all I need to extract is the changes to `generateAllInfo` 
> > > and `emitTargetCall` except I wouldn't have the `PresentModifierOnly` 
> > > restriction.  Of course, I would need to add tests.  Does all that sound 
> > > about right?  If so, I'll work on it.
> > Yes, something like this. Most probably, you won't need to add new tests, 
> > just modify the existing ones.
> OK, I'm working on this.  One point from the call I'm not confident about:  
> Is this change just for globals or locals too?  That is, which variables in 
> the following examples should have map entries?
> 
> ```
> int x;
> void fn() {
>   int y;
>   #pragma omp target map(x,y)
>   {
> // no references to x and y are lexically contained
>   }
> }
> ```
I think, both. We need to have references in the data environment for all 
explicitly mapped variables.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

ABataev wrote:
> jdenny wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > In today's OpenMP/LLVM call, it was pointed out that this issue 
> > > > represents a general Clang bug for map clauses: map clauses generally 
> > > > shouldn't be ignored just because their variables aren't referenced in 
> > > > the construct.
> > > > 
> > > > Here, I tried to handle this issue only in the case of a `present` 
> > > > modifier because I previously didn't consider whether the existing 
> > > > behavior for other map types was a bug.  My solution here was similar 
> > > > to what's done for `omp target data`, which doesn't have captures at 
> > > > all.  Perhaps, this should generalized to other map types.
> > > > 
> > > > Another possibility might be to expand what's captured in Sema.
> > > It must be fixed in a separate patch. We definitely should not modify 
> > > sema here, just the codegen. Currently, it iterates through all captures 
> > > in region, also need to iterate through the explicit maps.
> > Thanks for the response.  I'd be happy to generalize and extract into a new 
> > patch.
> > 
> > It looks like all I need to extract is the changes to `generateAllInfo` and 
> > `emitTargetCall` except I wouldn't have the `PresentModifierOnly` 
> > restriction.  Of course, I would need to add tests.  Does all that sound 
> > about right?  If so, I'll work on it.
> Yes, something like this. Most probably, you won't need to add new tests, 
> just modify the existing ones.
OK, I'm working on this.  One point from the call I'm not confident about:  Is 
this change just for globals or locals too?  That is, which variables in the 
following examples should have map entries?

```
int x;
void fn() {
  int y;
  #pragma omp target map(x,y)
  {
// no references to x and y are lexically contained
  }
}
```


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

jdenny wrote:
> ABataev wrote:
> > jdenny wrote:
> > > In today's OpenMP/LLVM call, it was pointed out that this issue 
> > > represents a general Clang bug for map clauses: map clauses generally 
> > > shouldn't be ignored just because their variables aren't referenced in 
> > > the construct.
> > > 
> > > Here, I tried to handle this issue only in the case of a `present` 
> > > modifier because I previously didn't consider whether the existing 
> > > behavior for other map types was a bug.  My solution here was similar to 
> > > what's done for `omp target data`, which doesn't have captures at all.  
> > > Perhaps, this should generalized to other map types.
> > > 
> > > Another possibility might be to expand what's captured in Sema.
> > It must be fixed in a separate patch. We definitely should not modify sema 
> > here, just the codegen. Currently, it iterates through all captures in 
> > region, also need to iterate through the explicit maps.
> Thanks for the response.  I'd be happy to generalize and extract into a new 
> patch.
> 
> It looks like all I need to extract is the changes to `generateAllInfo` and 
> `emitTargetCall` except I wouldn't have the `PresentModifierOnly` 
> restriction.  Of course, I would need to add tests.  Does all that sound 
> about right?  If so, I'll work on it.
Yes, something like this. Most probably, you won't need to add new tests, just 
modify the existing ones.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

ABataev wrote:
> jdenny wrote:
> > In today's OpenMP/LLVM call, it was pointed out that this issue represents 
> > a general Clang bug for map clauses: map clauses generally shouldn't be 
> > ignored just because their variables aren't referenced in the construct.
> > 
> > Here, I tried to handle this issue only in the case of a `present` modifier 
> > because I previously didn't consider whether the existing behavior for 
> > other map types was a bug.  My solution here was similar to what's done for 
> > `omp target data`, which doesn't have captures at all.  Perhaps, this 
> > should generalized to other map types.
> > 
> > Another possibility might be to expand what's captured in Sema.
> It must be fixed in a separate patch. We definitely should not modify sema 
> here, just the codegen. Currently, it iterates through all captures in 
> region, also need to iterate through the explicit maps.
Thanks for the response.  I'd be happy to generalize and extract into a new 
patch.

It looks like all I need to extract is the changes to `generateAllInfo` and 
`emitTargetCall` except I wouldn't have the `PresentModifierOnly` restriction.  
Of course, I would need to add tests.  Does all that sound about right?  If so, 
I'll work on it.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

jdenny wrote:
> In today's OpenMP/LLVM call, it was pointed out that this issue represents a 
> general Clang bug for map clauses: map clauses generally shouldn't be ignored 
> just because their variables aren't referenced in the construct.
> 
> Here, I tried to handle this issue only in the case of a `present` modifier 
> because I previously didn't consider whether the existing behavior for other 
> map types was a bug.  My solution here was similar to what's done for `omp 
> target data`, which doesn't have captures at all.  Perhaps, this should 
> generalized to other map types.
> 
> Another possibility might be to expand what's captured in Sema.
It must be fixed in a separate patch. We definitely should not modify sema 
here, just the codegen. Currently, it iterates through all captures in region, 
also need to iterate through the explicit maps.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added inline comments.



Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9566
+MEHandler.generateAllInfo(BasePointers, Pointers, Sizes, MapTypes,
+  CapturedVarSet, /*PresentModifierOnly=*/true);
 

In today's OpenMP/LLVM call, it was pointed out that this issue represents a 
general Clang bug for map clauses: map clauses generally shouldn't be ignored 
just because their variables aren't referenced in the construct.

Here, I tried to handle this issue only in the case of a `present` modifier 
because I previously didn't consider whether the existing behavior for other 
map types was a bug.  My solution here was similar to what's done for `omp 
target data`, which doesn't have captures at all.  Perhaps, this should 
generalized to other map types.

Another possibility might be to expand what's captured in Sema.


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

Ping.  Maybe we can start with the simplest question from the review summary:

> The Clang codegen test files are difficult to maintain because of their 
> sizes, but I tried to insert `present` tests into them anyway to be 
> consistent with the existing `always` and `close` tests. I'd like to move all 
> `present` codegen tests to separate test files. Any objections?


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

https://reviews.llvm.org/D83061



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

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny updated this revision to Diff 277215.
jdenny added a comment.

Rebased.


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

https://reviews.llvm.org/D83061

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_data_ast_print.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_parallel_map_messages.cpp
  clang/test/OpenMP/target_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -468,7 +468,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
@@ -543,7 +543,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
Index: clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
+++ clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
@@ -175,7 +175,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
@@ -287,7 +287,7 @@
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always: x) // expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
-#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target teams distribute simd map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
   for (i = 0; i < argc; ++i) foo();
 #pragma omp target teams distribute simd map(always, tofrom: always, tofrom, x)
   for (i = 0; i < argc; ++i) foo();
Index: clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
+++ clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
@@ -175,7 +175,7 @@
   for (i = 0; i < 

[PATCH] D83061: [OpenMP] Implement TR8 `present` map type modifier in Clang (1/2)

[Joel E. Denny via Phabricator via cfe-commits]

jdenny created this revision.
jdenny added reviewers: ABataev, jdoerfert, hfinkel, Meinersbur, kkwli0, 
grokos, sfantao, gtbercea.
Herald added subscribers: cfe-commits, sstefan1, guansong, yaxunl.
Herald added a project: clang.
jdenny added a parent revision: D83057: [OpenMP][NFC] Remove hard-coded line 
numbers from more tests.
jdenny removed a subscriber: llvm-commits.
jdenny added a reviewer: Hahnfeld.
jdenny added a child revision: D83062: [OpenMP] Implement TR8 `present` map 
type modifier in runtime (2/2).

This patch implements Clang front end support for the OpenMP TR8 
`present` map type modifier.  The next patch in this series implements
OpenMP runtime support.

This patch does not implement the `defaultmap` implicit behavior
`present`.

A `present` map type modifier behavior that this patch does not 
attempt to implement is TR8 sec. 2.22.7.1 "map Clause", p. 319,
L14-16:

> If a map clause with a present map-type-modifier is present in a map 
>  clause, then the effect of the clause is ordered before all other
>  map clauses that do not have the present modifier.

Compare to L10-11:

> For a given construct, the effect of a map clause with the to, from,
>  or tofrom map-type is ordered before the effect of a map clause with
>  the alloc, release, or delete map-type.

I have some questions before this patch is ready for a detailed
review:

1. Is either passage quoted above relevant for any case that does not involve 
aliasing?

2. As far as I can tell, Clang does not implement L10-11 to handle aliasing.  
Is it expected that it already does?  If not, then I think both passages should 
be implemented together later.  Any objections?

3. What should the order be in the case of `map(tofrom:expr1) 
map(present,alloc:expr2)`?

4. The Clang codegen test files are difficult to maintain because of their 
sizes, but I tried to insert `present` tests into them anyway to be consistent 
with the existing `always` and `close` tests.  I'd like to move all `present` 
codegen tests to separate test files.  Any objections?


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83061

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/include/clang/Basic/OpenMPKinds.def
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/test/OpenMP/target_ast_print.cpp
  clang/test/OpenMP/target_data_ast_print.cpp
  clang/test/OpenMP/target_data_codegen.cpp
  clang/test/OpenMP/target_defaultmap_codegen.cpp
  clang/test/OpenMP/target_enter_data_codegen.cpp
  clang/test/OpenMP/target_map_codegen.cpp
  clang/test/OpenMP/target_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_parallel_map_messages.cpp
  clang/test/OpenMP/target_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_parallel_for_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
  clang/test/OpenMP/target_teams_map_messages.cpp

Index: clang/test/OpenMP/target_teams_map_messages.cpp
===
--- clang/test/OpenMP/target_teams_map_messages.cpp
+++ clang/test/OpenMP/target_teams_map_messages.cpp
@@ -468,7 +468,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
@@ -543,7 +543,7 @@
 
 #pragma omp target data map(always, tofrom: x)
 #pragma omp target data map(always: x) // expected-error {{missing map type}}
-#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', or 'mapper'}} expected-error {{missing map type}}
+#pragma omp target data map(tofrom, always: x) // expected-error {{incorrect map type modifier, expected 'always', 'close', 'mapper', or 'present'}} expected-error {{missing map type}}
 #pragma omp target data map(always, tofrom: always, tofrom, x)
 #pragma omp target teams map(tofrom j) // expected-error {{expected ',' or ')' in 'map' clause}}
   foo();
Index: clang/test/OpenMP/target_teams_distribute_simd_map_messages.cpp
===
---