[PATCH] D120150: Constant folding of llvm.amdgcn.trig.preop

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:292
 
+static inline long as_long(double d) { union { double d; long u; } v; v.d = d; return v.u; }
+
----------------
Relying on size of double, also this is technically undefined.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:975
+
+    const ulong TwoByPi[] = {
+    0xa2f9836e, 0x4e441529, 0xfc2757d1, 0xf534ddc0,
----------------
uint32_t


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:991-992
+
+    if (const ConstantFP *Csrc = dyn_cast<ConstantFP>(Src)) {
+      if (const ConstantInt *Cseg = dyn_cast<ConstantInt>(Segment)) {
+
----------------
Early exit and reduce indentation


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:994
+
+      const APFloat &ArgVal = Csrc->getValueAPF();
+      double Dsrc = ArgVal.convertToDouble();
----------------
Indentation off


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1009
+      Thi >>= 11;
+      double Res = (double)Thi;
+      int Scale = -53 - Shift;
----------------
Should stick with apfloat operations


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1013
+        Scale += 128;
+      Res = ldexp(Res, Scale);
+      return IC.replaceInstUsesWith(II, ConstantFP::get(Src->getType(), Res));
----------------
You can use scalbn for APFloat instead of relying on host ldexp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120150