Constant folding of llvm.amdgcn.trig.preop

[arsenm]

Summary

If the parameters(the input and segment select) coming in to amdgcn.trig.preop intrinsic are compile time constants, then this patch pre-computes the output of amdgcn.trig.preop on the CPU and replaces the uses with the computed constant.

All the existing AMDGPU lit cases pass along with the negative cases where the parameters to this intrinsic are variable.
Added a simple test case with the exact output that matches the output from the GPU.

Created a small HIP test application with the exact compute logic(and the constants used for 2/pi) running on the CPU and the intrinsic invoked for the GPU kernel.
Ran the test over the entire range of double floating-point. The outputs from the CPU and those from the intrinsic on gfx10 AMD GPU match.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

292

Relying on size of double, also this is technically undefined.

975

uint32_t

991–992

Early exit and reduce indentation

994

Indentation off

1009

Should stick with apfloat operations

1013

You can use scalbn for APFloat instead of relying on host ldexp

[foad]

Ran the test over the entire range of double floating-point.

Just curious: how long does it take to test all 2^64 inputs?

[b-sumner]

In D120150#3332613, @foad wrote:

Ran the test over the entire range of double floating-point.

Just curious: how long does it take to test all 2^64 inputs?

The only dependence is on the exponent so 2K inputs is sufficient.

[Ravi]

In D120150#3332613, @foad wrote:

Ran the test over the entire range of double floating-point.

Just curious: how long does it take to test all 2^64 inputs?

It took around 5 days on Ryzen 9 - 5950. Was running a single thread for CPU though. And each input was checked with all 32 segments.

[Ravi]

In D120150#3332632, @b-sumner wrote:

In D120150#3332613, @foad wrote:

Ran the test over the entire range of double floating-point.

Just curious: how long does it take to test all 2^64 inputs?

The only dependence is on the exponent so 2K inputs is sufficient.

Yes right. That would have greatly reduced the run times. Missed out, didn't give a thought in that direction.

[Ravi]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1009

Yes, will do that.

1013

Yes..will try it out and check for any precision difference. Should be good as long as the internal implementation of APFloat is within 0.5 ULP.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1013

Actually we should be constant folding the ldexp intrinsic too. I thought I did that before, but in the code above I don't see it handling arbitrary constants

[Ravi]

Fixed all the review comments.

[Ravi]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1013

Can be done in another patch

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

989

demorgan this

1000

extra parentheses

1007

extra parentheses

1019

extra parentheses

1020

extra parentheses

[foad]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

986

Capitalizing the names like CSrc, CSeg, FSrc, NumBits etc is more common.

994

Just use ->getValue. And in general I think you should convert CSrc and CSeg to plain uint32_t / uint64_t as soon as possible. You have lots of uses of APInt below where it really is not necessary.

[arsenm]

Reverse ping

[arsenm]

@Ravi where are these tests you mentioned?

[arsenm]

llvm/test/Transforms/InstCombine/AMDGPU/amdgcn-intrinsics.ll

5638 ↗

(On Diff #475913)

I feel like the tests are lacking in sample values but coming up with ones to test every point in the table seems exhausting

[arsenm]

Add assert, which shows this table lookup is broken.

Also improve poison handling

[foad]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1019

This (and some of the calculation below) can just use int. No need for APInt.

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

return modifyIntrinsicCall(
         // Convert the bias
         if (!OnlyDerivatives && ImageDimIntr->NumBiasArgs != 0) {
           Value *Bias = II.getOperand(ImageDimIntr->BiasIndex);
           Args[ImageDimIntr->BiasIndex] = convertTo16Bit(*Bias, IC.Builder);
         }
       });
 }
 
+static inline long as_long(double d) { union { double d; long u; } v; v.d = d; return v.u; }
+
 bool GCNTTIImpl::canSimplifyLegacyMulToMul(const Value *Op0, const Value *Op1,
                                            InstCombiner &IC) const {
   // The legacy behaviour is that multiplying +/-0.0 by anything, even NaN or
   // infinity, gives +0.0. If we can prove we don't have one of the special
   // cases then we can use a normal multiply instead.
   // TODO: Create and use isKnownFiniteNonZero instead of just matching
   // constants here.
   if (match(Op0, PatternMatch::m_FiniteNonZero()) ||
case Intrinsic::amdgcn_ldexp: {
     // ldexp(x, 0) -> x
     // ldexp(x, undef) -> x
     if (isa<UndefValue>(Op1) || match(Op1, PatternMatch::m_ZeroInt())) {
       return IC.replaceInstUsesWith(II, Op0);
     }
 
     break;
   }
+  case Intrinsic::amdgcn_trig_preop: {
+
+    const ulong TwoByPi[] = {
+    0xa2f9836e, 0x4e441529, 0xfc2757d1, 0xf534ddc0,
+    0xdb629599, 0x3c439041, 0xfe5163ab, 0xdebbc561,
+    0xb7246e3a, 0x424dd2e0, 0x06492eea, 0x09d1921c,
+    0xfe1deb1c, 0xb129a73e, 0xe88235f5, 0x2ebb4484,
+    0xe99c7026, 0xb45f7e41, 0x3991d639, 0x835339f4,
+    0x9c845f8b, 0xbdf9283b, 0x1ff897ff, 0xde05980f,
+    0xef2f118b, 0x5a0a6d1f, 0x6d367ecf, 0x27cb09b7,
+    0x4f463f66, 0x9e5fea2d, 0x7527bac7, 0xebe5f17b,
+    0x3d0739f7, 0x8a5292ea, 0x6bfb5fb1, 0x1f8d5d08,
+    0x56033046
+    };
+
+    Value *Src = II.getArgOperand(0);
+    Value *Segment = II.getArgOperand(1);
+
+    if (const ConstantFP *Csrc = dyn_cast<ConstantFP>(Src)) {
+      if (const ConstantInt *Cseg = dyn_cast<ConstantInt>(Segment)) {
+
+      const APFloat &ArgVal = Csrc->getValueAPF();
+      double Dsrc = ArgVal.convertToDouble();
+      const APInt &SegVal = Cseg->getUniqueInteger();
+      int Iseg = SegVal.getSExtValue();
+
+      const int Eclamp = 1077;
+      int E = (as_long(Dsrc) >> 52) & 0x7ff;
+      int Shift = (E > Eclamp ?  E - Eclamp : 0) + 53 * Iseg;
+      int I = Shift >> 5;
+      int Bshift = Shift & 0x1f;
+      ulong Thi = (TwoByPi[I] << 32) | TwoByPi[I+1];
+      ulong Tlo = TwoByPi[I+2] << 32;
+      if (Bshift > 0)
+        Thi = (Thi << Bshift) | (Tlo >> (64-Bshift));
+      Thi >>= 11;
+      double Res = (double)Thi;
+      int Scale = -53 - Shift;
+      if (E >= 0x7b0)
+        Scale += 128;
+      Res = ldexp(Res, Scale);
+      return IC.replaceInstUsesWith(II, ConstantFP::get(Src->getType(), Res));
+      }
+    }
+    break;
+  }
   case Intrinsic::amdgcn_fmul_legacy: {
     Value *Op0 = II.getArgOperand(0);
     Value *Op1 = II.getArgOperand(1);
 
     // The legacy behaviour is that multiplying +/-0.0 by anything, even NaN or
     // infinity, gives +0.0.
     // TODO: Move to InstSimplify?
     if (match(Op0, PatternMatch::m_AnyZeroFP()) ||
         match(Op1, PatternMatch::m_AnyZeroFP()))
       return IC.replaceInstUsesWith(II, ConstantFP::getNullValue(II.getType()));

llvm/test/CodeGen/AMDGPU/llvm.amdgcn.trig.preop.ll

 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -verify-machineinstrs < %s | FileCheck -check-prefix=SI %s
 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=tonga -mattr=-flat-for-global -verify-machineinstrs < %s | FileCheck -check-prefix=SI %s
+; RUN:  opt -S -mtriple=amdgcn-- -mcpu=gfx900 -instcombine < %s | FileCheck -check-prefix=GCN %s
+; RUN:  opt -S -mtriple=amdgcn-- -mcpu=gfx1010 -instcombine < %s | FileCheck -check-prefix=GCN %s
 
 declare double @llvm.amdgcn.trig.preop.f64(double, i32) nounwind readnone
 
 ; SI-LABEL: {{^}}test_trig_preop_f64:
 ; SI-DAG: buffer_load_dword [[SEG:v[0-9]+]]
 ; SI-DAG: buffer_load_dwordx2 [[SRC:v\[[0-9]+:[0-9]+\]]],
 ; SI: v_trig_preop_f64 [[RESULT:v\[[0-9]+:[0-9]+\]]], [[SRC]], [[SEG]]
 ; SI: buffer_store_dwordx2 [[RESULT]],

 ; SI: buffer_store_dwordx2 [[RESULT]],
 ; SI: s_endpgm
 define amdgpu_kernel void @test_trig_preop_f64_imm_segment(double addrspace(1)* %out, double addrspace(1)* %aptr) nounwind {
   %a = load double, double addrspace(1)* %aptr, align 8
   %result = call double @llvm.amdgcn.trig.preop.f64(double %a, i32 7) nounwind readnone
   store double %result, double addrspace(1)* %out, align 8
   ret void
 }
+
+define protected amdgpu_kernel void @trig_preop_constfold(double addrspace(1)* nocapture %0, double addrspace(1)* nocapture readnone %1, i32 %2){
+; GCN: store double 0x2F42371D2126E970, double addrspace(1)* %0, align 8
+; GCN-NEXT: ret void
+  %4 = tail call contract double @llvm.amdgcn.trig.preop.f64(double 3.454350e+02, i32 5)
+  store double %4, double addrspace(1)* %0, align 8
+  ret void
+}