[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:
> 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?
I added a test for this in `begin_declare_variant_messages.c` and confirmed my earlier statement. Then I implemented what you describe above (in spirit).

> 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?

That would potentially work but require us to touch a lot of places that deal with redefinitions and overload resolution. Getting the right clashes between specialized versions with the same name and openmp context but not getting the clashes for the same name but different openmp context will be non-trivial. Similarly, we need to ignore specialized versions during overload resolution, etc. etc.

FWIW, this is design 8 or 9, or even more. I tried to keep the names till codegen in the beginning, using an approach similar to what you described. I tried to use namespaces (even in C) to get the right kind of name clashes and interactions, I tried ... This is the only thing that (so far) worked reliably and is (IMHO) very non-intrusive.


> Also, these can have external linkage, right?

Yes. My name clash test has 3 different linkages, see `begin_declare_variant_messages.c`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75779