[Openmp-commits] [PATCH] D75779: [OpenMP] `omp begin/end declare variant` - part 2, sema (+"CG")

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5396
+  IdentifierInfo &VariangtII = Context.Idents.get(
+      (D.getIdentifier()->getName() + "." + DVScope.NameSuffix).str());
+  D.SetIdentifier(&VariangtII, D.getBeginLoc());
----------------
hfinkel wrote:
> Is there any way in which this name might become visible to users (e.g., in error messages)?
Yes, I think so. One way to trigger it would be to define the same function in the same `omp begin declare variant scope` (or two with the same context). I haven't verified this though.
TBH, I'm unsure how bad this actually is in the short term. The original name is still a at the beginning. We should obviously make sure the error message is appropriate eventually, e.g., it de-mangles the name.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5471
+      // variant expression. We allow this to fail in which case we continue with
+      // the next best variant expression.
+      Sema::TentativeAnalysisScope Trap(*this);
----------------
hfinkel wrote:
> Please provide an example or otherwise explain why this failure behavior is correct.
Tried to improve the comment.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487
+
+    VMIs.erase(VMIs.begin() + BestIdx);
+    Exprs.erase(Exprs.begin() + BestIdx);
----------------
hfinkel wrote:
> Why is the ordering here useful? Don't you collect all of the variant clauses from all of the declarations? Can't there be duplicates? (and does the relative order need always be the same?) Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the `while (CalleeFnDecl)` loop, but this makes me wonder if that would still have an odd behavior.
> Why is the ordering here useful?

I don't think it is ordered. We have a conceptual set and BestIdx determines the which of the set is the best. All others are equal.

> Don't you collect all of the variant clauses from all of the declarations? 

Yes.

> Can't there be duplicates? (and does the relative order need always be the same?)

Yes (and no).  `getBestVariantMatchForContext` should determine the best regardless of duplicates, we might just try it multiple times if we didn't manage to create a call.

> Are we effectively supporting here, as an extension, cases where not all of the declarations have the same set of variants declared (it loos like we are because there's no break in the while (CalleeFnDecl) loop, but this makes me wonder if that would still have an odd behavior.

We are supporting that. All declarations are scanned and all variants are collected. What odd behavior do you refer to?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75779