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

Mon Jul 20 21:16:28 PDT 2020

cameron.mcinally added inline comments.

================
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
----------------
cameron.mcinally wrote:
> 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...
This problem is really sticky (and maybe a design flaw in DAGCombine). The initial DAGs for pre/post-Diff are largely the same, except for the FSUB/FNEG node. 

The problem arrises in visitFSUB(...), when the FSUB(-0,X) is transformed to FNEG(X). This combine, of course, removes the FSUB(-0,X) from its current position in the Worklist, and the new FNEG(X) is placed at the end of the Worklist.

This new Worklist order is pretty unfortunate. The two transforms that are needed to fold the FNEG into the FMED3 are now out of order. (If DAGCombine ran one more time, it looks like we'd correctly get the fold.)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84056