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