[Openmp-commits] [PATCH] D77252: [OpenMP] Try to find an existing base for `omp begin/end declare variant`

Sat Apr 4 17:01:56 PDT 2020

jdoerfert marked 2 inline comments as done.
jdoerfert added inline comments.

================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5512
+
+  if (!BaseFD) {
+    // TODO: Determine if we can reuse the declarator to create a declaration
----------------
mikerice wrote:
> I think if you do the base lookup in ActOnStartOfFunctionDefinitionInOpenMPDeclareVariantScope instead of here you might have better success using the declarator.  You would need to get the type and  constexpr/consteval info from the declarator and declspec but if the lookup fails you can just use ActOnDeclarator to get a base declaration like you were before.  I tried this in my sandbox and it seems to work (AST tests fail I'm guessing that is just order change).  If that works we could get rid of these hacky bits.
Can you share how you do the lookup and the type comparison with the Declarator? I mean, name lookup is simple but determining the right overload wrt. type and the namespace was something I always had problems with. If you want to stress test your solution and don't mind, take the math patch and run the `https://github.com/TApplencourt/OmpVal` tests:
`CXX='clang++' CXXFLAGS='-fopenmp -fopenmp-targets=nvptx64-nvidia-cuda' ./omphval.sh run ./tests/math_cpp11`
You'll see compile or link errors if it doesn't work for any reason. Doesn't mean we could not make it work by changing in the math wrappers but that depends on the errors (if any).

================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5551
+  BaseFD->addAttr(OMPDeclareVariantA);
   BaseFD->setImplicit(true);
 }
----------------
mikerice wrote:
> BaseFD is only implicit now if you created it, not when one was found in the lookup case right? 
Correct. Need to be changed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77252