[PATCH] D105981: [AMDGPU][OpenMP] Support linking of math libraries

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:879-889
+  // TODO: There are way too many flags that change this. Do we need to check
+  // them all?
+  bool DAZ = DriverArgs.hasArg(options::OPT_cl_denorms_are_zero) ||
+             getDefaultDenormsAreZeroForTarget(Kind);
+  bool FiniteOnly = DriverArgs.hasArg(options::OPT_cl_finite_math_only);
+
+  bool UnsafeMathOpt =
----------------
yaxunl wrote:
> I agree that we'd better refactor this part as a common function used by HIP/OpenMP/OpenCL. However I disagree to let HIP use OpenCL options. HIP uses clang generic options or HIP/CUDA shared options to control these flags. I think OpenMP may consider using similar options as HIP instead of OpenCL options.
It's a mixture. Some flags look generic, some have hip in the name. My preference would be to have a generic named flag for each one, and alias hip/opencp/cuda onto the generic one.

Aliasing the flags would mean what works today continues working while simplifying the control flow in clang. Keeping flags with different names that do the same thing is certainly possible but doesn't seem a feature. It makes clang more complicated in order to make user build scripts more complicated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105981