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

329

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

1010

uint32_t

1026–1027

Early exit and reduce indentation

1029

Indentation off

1044

Should stick with apfloat operations

1048

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

1044

Yes, will do that.

1048

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

1048

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

1048

Can be done in another patch

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1024

demorgan this

1035

extra parentheses

1042

extra parentheses

1054

extra parentheses

1055

extra parentheses

[foad]

llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp

1021

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

1029

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

1054

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);
         }
       });
 }
 
 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()) ||
       match(Op1, 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 uint32_t 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);
+    const ConstantFP *Csrc = dyn_cast<ConstantFP>(Src);
+    const ConstantInt *Cseg = dyn_cast<ConstantInt>(Segment);
+
+    if (!(Csrc && Cseg))
+      break;
+
+    const APFloat &Fsrc = Csrc->getValueAPF();
+
+    const APInt &SegVal = Cseg->getUniqueInteger();
+    bool Ovflow;
+    unsigned Numbits = 32;
+    bool Signed = true;
+
+    APInt EClamp(Numbits, 1077, Signed);
+    APInt E = (Fsrc.bitcastToAPInt()).ashr(52);
+    E &= 0x7ff;
+    E = E.trunc(Numbits);
+    APInt Shift =
+        (E.sgt(EClamp) ? E.ssub_ov(EClamp, Ovflow) : APInt(Numbits, 0, Signed))
+            .sadd_ov(APInt(Numbits, 53, Signed).smul_ov(SegVal, Ovflow),
+                     Ovflow);
+    int32_t I = (Shift.ashr(5)).getSExtValue();
+    APInt Bshift = Shift & 0x1f;
+    Numbits = 64;
+    Signed = false;
+    APInt Thi = APInt(Numbits,
+                      (((uint64_t)TwoByPi[I] << 32) | (uint64_t)TwoByPi[I + 1]),
+                      Signed);
+    APInt Tlo = APInt(Numbits, ((uint64_t)TwoByPi[I + 2] << 32), Signed);
+
+    if (Bshift.sgt(0)) {
+      Numbits = 32;
+      Signed = true;
+      Thi = (Thi.shl(Bshift)) |
+            (Tlo.lshr(APInt(Numbits, 64, Signed).ssub_ov(Bshift, Ovflow)));
+    }
+
+    Thi = Thi.lshr(11);
+    APFloat Res = APFloat(Thi.roundToDouble());
+    int32_t Scale = -53 - Shift.getSExtValue();
+
+    if (E.sge(0x7b0))
+      Scale += 128;
+
+    Res = scalbn(Res, Scale, RoundingMode::NearestTiesToEven);
+    double Resd = Res.convertToDouble();
+    return IC.replaceInstUsesWith(II, ConstantFP::get(Src->getType(), Resd));
+  }
   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()) ||

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
+}