[PATCH] D84056: [FPEnv] Don't transform FSUB(-0, X) -> FNEG(X) in SelectionDAGBuilder.

Mon Jul 20 07:24:08 PDT 2020

cameron.mcinally added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:9274-9275
+  case ISD::FSUB: {
+    // FSUB(-0.0,X) can be lowered or combined as a bit operation.
+    // Need to check its input recursively to handle.
+    ConstantFPSDNode *N0C = isConstOrConstSplatFP(Op.getOperand(0), true);
----------------
arsenm wrote:
> I thought the point was this cannot be lowered this way?
This isn't as bad as it seems, I think. This is `isCanonicalized(...)`, so we're just saying the the FSUB(-0,X) *might* not flush denormals (if DAGCombine wants to change it to an FNEG(X) later). 

We could check the DenormalMode here though. That would let us know what will happen to the FSUB(-0,X) in DAGCombine. Assuming it's not a can of worms, I could do that. If it is a can-of-worms, might be better left for separate patch. It should be functional with this patch, just not optimal.

================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.fmed3.ll:39
   %med3 = call float @llvm.amdgcn.fmed3.f32(float %src0, float %src1, float %src2)
   %neg.med3 = fsub float -0.0, %med3
   %med3.user = fmul float %med3, 4.0
----------------
arsenm wrote:
> This is a code size regression. We probably need 
> 
> This should just change to use fneg. We could probably consider the fsub case in performFNegCombine though
> This should just change to use fneg.

Are you saying the IR in this test should just be updated to use FNEG?

I'll look to see if there's a clean way to combine FSUB with the FMED...

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84056