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

[Hal Finkel via Phabricator via Openmp-commits]

hfinkel 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());
----------------
jdoerfert wrote:
> 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.
I think that we have to consider diagnostic quality as a first-class citizen. There are a few different ways that we might approach this. A minimal diff from what you have might be:

  1. Replace the "." above with ".ompvariant."
  2. Add logic to FunctionDecl::getNameForDiagnostic (or NamedDecl::getNameForDiagnostic) that recognizes that magic string in the name and alters the printing to do something intelligible for the user with the name.

(I think that (1) is a good idea anyway).

Another way would be to add some first-class property to the function and then leave the mangling to CodeGen. Are we mangling these in Sema in other places too?




Also, these can have external linkage, right?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5487
+
+    VMIs.erase(VMIs.begin() + BestIdx);
+    Exprs.erase(Exprs.begin() + BestIdx);
----------------
jdoerfert wrote:
> 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?
> 
Makes sense, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75779