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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits