[PATCH] D80702: [AMDGPU] Fold llvm.amdgcn.cos and llvm.amdgcn.sin intrinsics

Wed Jun 3 03:48:21 PDT 2020

foad marked 2 inline comments as done.
foad added inline comments.

================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:1918
+        auto NativeFP = IntrinsicID == Intrinsic::amdgcn_cos ? cos : sin;
+        return ConstantFoldFP(NativeFP, V * 2 * M_PI, Ty);
     }
----------------
arsenm wrote:
> foad wrote:
> > arsenm wrote:
> > > arsenm wrote:
> > > > b-sumner wrote:
> > > > > arsenm wrote:
> > > > > > foad wrote:
> > > > > > > arsenm wrote:
> > > > > > > > foad wrote:
> > > > > > > > > This folds amdgcn.sin(1.0) to a very small value that is not exactly 0.0. Should I add an explicit check for all of the quarter-integer values that should fold to exactly -1.0 or 0.0 or +1.0 ?
> > > > > > > > I've never been sure what exactly the policy should be for folding these sorts of intrinsics. I think nobody is happy with relying on the host libm call to begin with, plus the hardware instructions don't give the exact same results. We're already constant folding more precise results for rcp than the hardware instruction gives though (which is probably more useful in general).
> > > > > > > > 
> > > > > > > > I'm not sure this is how this will work going forward, but you could also consider strictfp on the call site (it may happen already, but you should add a test where certain folds are skipped)
> > > > > > > > 
> > > > > > > > Also you should use numbers::pi rather than relying on M_PI (and I strongly prefer using an explicit 2.0 rather than 2)
> > > > > > > > you could also consider strictfp on the call site
> > > > > > > 
> > > > > > > I thought:
> > > > > > > - strictfp is to do with whether certain math functions are guaranteed to respect the ambient rounding mode etc
> > > > > > > - the definition of amdgcn.cos is "do what the hardware instruction does"
> > > > > > > - the hardware instruction doesn't behave differently depending on the rounding mode
> > > > > > > - so there's no need to worry about strictfp
> > > > > > > But perhaps I'm wrong about some or all of this.
> > > > > > I'm pretty sure the nan quieting behavior will change based on the ieee_mode bit. AMDGPU has a wider set of FP environment properties. Plus, these support exceptions anyway
> > > > > I don't understand the first comment above where the argument is 1.0.  These intrinsics take radians, and not the scaled values the ISA expects,, correct?  Similarly, if any bounds checking is done, shouldn't it be on radians?
> > > > From the lowering it looks like they directly feed the subtarget instruction. The lowering for llvm.sin/llvm.cos inserts the scale or not depending on the target (I'm not sure why you would ever need the raw amdgcn intrinsic form)
> > > The device libraries have one function that uses both in the raw form, and doesn't consider the two subtarget behaviors
> > LLPC sometimes generates the amdgcn.sin/cos intrinsics. The problem with introducing the multiply when llvm.sin/cos is lowered is that it doesn't get folded away if the argument was already something multiplied or divided by a constant, which is pretty common.
> It should in the DAG lowering. We're not propagating fast math flags now in the sin lowering, maybe that's your problem?
Good point, thanks, see D80813. Perhaps there is no good reason for LLPC to use the amdgcn intrinsics after all.

================
Comment at: llvm/test/Analysis/ConstantFolding/AMDGPU/sin.ll:138
+; CHECK-NEXT:    store volatile double 0.000000e+00, double* [[P]], align 8
+; CHECK-NEXT:    store volatile double 0x3FE6A09E667F3BCC, double* [[P]], align 8
+; CHECK-NEXT:    store volatile double 0xBFE6A09E667F3BCC, double* [[P]], align 8
----------------
thakis wrote:
> Looks like this ends in BCD instead of BCC on Windows (with clang-cl as host compiler at least): http://45.33.8.238/win/16679/step_11.txt
Thanks, should be fixed in rGc27214c23446e423ec2e7eb8650a65cc5f0a16aa.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80702