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

Tue Jul 21 08:57:29 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);
----------------
cameron.mcinally wrote:
> 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.
> 
> 
> We could check the DenormalMode here though. 

Guarding this code based on denormal mode is easy enough, but it affects the assembly in a number of tests where denormals are enabled. That will make this patch harder to review. It might be better to optimize in a stand-alone Diff.

Thoughts on this?

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

https://reviews.llvm.org/D84056