[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<OpenMPMapModifierKind>(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