[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/AST/OpenMPClause.cpp:1723
+        llvm::APInt CondVal =
+            Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx);
+        if (CondVal.isNullValue())
----------------
aaron.ballman wrote:
> jdoerfert wrote:
> > aaron.ballman wrote:
> > > I didn't see anything that validated that this is known to be a constant expression. Did I miss something?
> > > 
> > > Also, is there anything checking that the expression is not value dependent?
> > > I didn't see anything that validated that this is known to be a constant expression. Did I miss something? 
> > 
> > I'll look into that and make sure we check in the parser.
> > 
> > > Also, is there anything checking that the expression is not value dependent?
> > 
> > That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?
> > That should be fine. If it is attached to a template it is instantiated with the template. Or am I missing something?
> 
> `EvaluateKnownConstInt()` asserts that the expression is not value dependent, so I just wanted to be certain that assertion wouldn't trigger. Sounds like it won't because this should only be called on an instantiation?
I added detection and tests for constant, value-dependent, and non-constant expressions in score and user condition. Assuming I didn't miss anything it should not assert here.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1379
   // Parse inner context selectors.
-  SmallVector<Sema::OMPCtxSelectorData, 4> Data;
-  if (!parseOpenMPContextSelectors(Loc, Data)) {
-    // Parse ')'.
-    (void)T.consumeClose();
-    // Need to check for extra tokens.
-    if (Tok.isNot(tok::annot_pragma_openmp_end)) {
-      Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
-          << getOpenMPDirectiveName(OMPD_declare_variant);
-    }
-  }
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  parseOMPContextSelectors(Loc, *TI);
----------------
jdoerfert wrote:
> aaron.ballman wrote:
> > What is responsible for freeing this memory?
> Will check and fix if needed.
I made sure all OMPTraitInfo are freed, I think. If an `OMPDeclareVariantAttr` is build its taking owenership and calls delete in the destructor.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:110
       .Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
+      .EndsWith("*", "Record.readUserType<" +
+                         std::string(type.data(), 0, type.size() - 1) + ">()")
----------------
aaron.ballman wrote:
> jdoerfert wrote:
> > aaron.ballman wrote:
> > > The idea here being that if someone adds a `GenericPointerArg`, they'll have to add a specialization of `readUserType<>` for that named pointer type or else get template instantiation errors because there's no definition for the primary template?
> > Correct.
> I like it. :-)
It's gone now ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71830