[PATCH] D108380: [openmp][nfc] Refactor GridValues

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield added inline comments.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPGridValues.h:102
+    return R;
+  }
 };
----------------
jdoerfert wrote:
> JonChesterfield wrote:
> > jdoerfert wrote:
> > > It should be in the device rtl then, no?
> > This header is currently used from clang and the (amdgpu, could also be cuda if we like) host plugin. Possibly also from llvm. As of D108391 it would be used from the devicertl.
> > 
> > The idea is to have a single source of truth for the various magic numbers that the pieces should agree on and llvm is the common point on the dependency tree. I'm currently interested in that because I want to change some of them for gfx10 and have that magically ripple through the components. I'm not totally confident that will work out nicely for the host plugin as it has to dynamically handle different architectures but I think it'll be good enough.
> > 
> > It's not totally ideal to hand spin a function that is in the math support header but I also don't want to try to make various llvm headers ffreestanding-safe.
> >  It's not totally ideal to hand spin a function that is in the math support header but I also don't want to try to make various llvm headers ffreestanding-safe.
> 
> The function is only needed in the device rtl. Put it in the device rtl.
This particular function is only called by warpSizeLog2 which is currently only used by CGOpenMPRuntimeGPU. The deviceRTL doesn't call the function. However if this header includes the rest of llvm support then it can't call any of the others either.

I'm going to drop the log2 accessor (and this function) in favour of two calls into math support from CGOpenMPRuntime.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108380