[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Johannes Doerfert via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/AST/StmtOpenMP.cpp:2243
 }
+
+// TODO: We have various representations for the same data, it might help to
----------------
ABataev wrote:
> jdoerfert wrote:
> > This code was basically only moved, not written for this patch. It needs to life somewhere accessible from Parser to CodeGen, see the TODOs below.
> I don't think this is the right place for this code. Will try to move it to Basic directory in my patch.
Sure. As noted in the TODOs, finding a place for this is needed.


================
Comment at: clang/lib/Headers/__clang_cuda_cmath.h:70
 __DEVICE__ float fmod(float __x, float __y) { return ::fmodf(__x, __y); }
-// TODO: remove when variant is supported
-#ifndef _OPENMP
----------------
JonChesterfield wrote:
> jdoerfert wrote:
> > As far as I can tell, `fpclassify` is not available in CUDA so it is unclear if we want to have it here or not. I removed it due to the TODO above. Consequently I also had to remove other `fpclassify` occurrences. If it turns out the host version is not usable on the device and we need the builtins, we add them back but under the opposite guard, that is `#ifdef _OPENMP`.
> We could call __builtin_fpclassify for nvptx, e.g. from https://github.com/ROCm-Developer-Tools/aomp-extras/blob/0.7-6/aomp-device-libs/libm/src/libm-nvptx.cpp
> 
> ```int fpclassify(float __x) {
>   return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, FP_ZERO, __x);
> }
> int fpclassify(double __x) {
>   return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, FP_ZERO, __x);
> }
> ```
Agreed. Assuming it works, I'll put the fpclassify code back in but only remove the todo and OPENMP macro.


================
Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_math_declares.h:17
 
 #if defined(__NVPTX__) && defined(_OPENMP)
 
----------------
hfinkel wrote:
> Should we use a more-specific selector and then get rid of this `__NVPTX__` check?
For now, this is CUDA after all. I was going to revisit this once we know how the AMD solution looks (I guess via HIP).
That said, I'd put a pin on it for now.

(The `kind(gpu)` selector below is only because we don't have anything more specific and it matches all our one GPU targets for now.)


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1489
+        ++Nesting;
+    } while (Nesting);
+
----------------
ABataev wrote:
> hfinkel wrote:
> > Will this just inf-loop if the file ends?
> It will.
We'll add a check and test.


================
Comment at: clang/test/OpenMP/begin_declare_variant_codegen.cpp:71
+}
+
+// Make sure all ompvariant functions return 1 and all others return 0.
----------------
JonChesterfield wrote:
> The name mangling should probably append the device kind, .e.g. `_Z3foov.ompvariant.gpu`
There is already a TODO for that (I think CodeGenModule). Mangling right now is hardcoded and needs to be revisited :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179