[Openmp-commits] [PATCH] D60906: [OpenMP][libomptarget] Add math functions support in OpenMP offloading

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/interface.h:585
+EXTERN double __kmpc_logb(double);
+EXTERN float __kmpc_logbf(float);
+
----------------
Shouldn't we do this with macros again?

I would even propose a separate "math_macro.inc" file that is included
in both places. In one the macro `DECL_ONLY` is set and we get only declarations
while in the other we expand to definitions. 
The idea is we simplify the maintenance in the future and cut down code.
Finally, we probably want to reuse the "math_macro.inc" also in other deviceRTLs so
we always support the same functions across all targets. Does that makes sense?


================
Comment at: libomptarget/deviceRTLs/nvptx/src/math.cu:21
+    return __fn(__x);                                                          \
+  }
+
----------------
I was thinking we have some macro for float/double generation:

```
#define __OPENMP_MATH_FUNC_1_FP(__fn, __kmpc_fn)   \
   __OPENMP_MATH_FUNC_1(float, __fn, __kmpc_fn) \
   __OPENMP_MATH_FUNC_1(double, __fn, __kmpc_fn) \
```

so we can cut down further below


================
Comment at: libomptarget/deviceRTLs/nvptx/src/math.cu:29
+
+__OPENMP_MATH_FUNC_2(float, powf, __kmpc_powf);
+__OPENMP_MATH_FUNC_2(double, pow, __kmpc_pow);
----------------
I don't think we want the semicolon at the end.


================
Comment at: libomptarget/deviceRTLs/nvptx/src/math.cu:34
+  return pow((double) a, (double) b);
+}
+
----------------
Wouldn't we get the correct conversion for:
  `__OPENMP_MATH_FUNC_2(long double, pow, __kmpc_powl);`
If not, should we add a two type version that casts the arguments
and define `__OPENMP_MATH_FUNC_2` in terms of that one?


Repository:
  rOMP OpenMP

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

https://reviews.llvm.org/D60906