[PATCH] D73978: [WIP][FPEnv] Don't transform FSUB(-0.0,X)->FNEG(X) when flushing denormals

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2997-3015
   // -0.0 - X --> fneg
   Type *Ty = I.getType();
   if (isa<Constant>(I.getOperand(0)) &&
       I.getOperand(0) == ConstantFP::getZeroValueForNegation(Ty)) {
     SDValue Op2 = getValue(I.getOperand(1));
-    setValue(&I, DAG.getNode(ISD::FNEG, getCurSDLoc(),
-                             Op2.getValueType(), Op2));
-    return;
+    EVT VT =  Op2.getValueType();
+
----------------
cameron.mcinally wrote:
> spatel wrote:
> > arsenm wrote:
> > > spatel wrote:
> > > > cameron.mcinally wrote:
> > > > > cameron.mcinally wrote:
> > > > > > spatel wrote:
> > > > > > > cameron.mcinally wrote:
> > > > > > > > arsenm wrote:
> > > > > > > > > arsenm wrote:
> > > > > > > > > > Why does SelectionDAGBuilder bother doing this fold at all? It should just directly translate the fsub?
> > > > > > > > > I think this should be just ripped out in a separate patch. The same problem seems to have been copied to IRTranslator, which should also be removed
> > > > > > > > @sanjay, what do you think? Seems reasonable to me. 
> > > > > > > > 
> > > > > > > > I think it made sense to do this when FNEG(X) was the canonical form of FSUB(-0.0, X). Wouldn't want two forms floating around for even a small amount of time. But now that there are cases where the operations are distinct through llc, it seems ok to wait until DAGCombine.
> > > > > > > Yes, now that we can use DenormMode to distinguish target behavior, it seems better to do it later in DAGCombiner if that would make sense for the target.
> > > > > > I'll have to put a pin in this for now. Removing this block is causing regressions in about 15 tests. The regressions appear to be subtle lowering differences, so I suspect it will take some time to straighten them out.
> > > > > I looked into removing this and there are warts underneath. Some are surmountable (different lowerings), but one is worrisome. I.e. the case where the FNeg operand is undef:
> > > > > 
> > > > > FNEG(undef) -> undef
> > > > > FSUB(-0.0, undef) -> NaN
> > > > > 
> > > > > That is, removing this transform propagates NaNs where we previously had undef values.
> > > > > 
> > > > > Any thoughts on how to proceed? Do we want to minimize code differences by keeping this transform in place? Or are we okay moving forward with the undef->NaN change?
> > > > Did that difference show up as real regressions or something benign? 
> > > > We could add a special-case fold for this here or getNode() if it helps:
> > > > fsub C, undef --> undef (as long as C is not NaN or Inf?)
> > > This sounds more correct to me. I don't see why this would be special cased
> > It's a special-case in the sense that folding to NaN is correct in general. Just dealing with this particular pattern is also a special-case because we could do something similar for all FP ops, not just fsub with constant operand 0. But we'll need to work out if/how the corner cases differ per opcode.
> Benign llc regression tests. The NaN and undef propagate differently, so the asm differences appear worse than they are.
> 
> > We could add a special-case fold for this here or getNode() if it helps:
> > fsub C, undef --> undef (as long as C is not NaN or Inf?)
> 
> That's a good idea. I don't feel strongly about it, but the current transform might be more obvious than adding a special case fold though.
> 
> 
I tried to generalize this over in D74713, but there doesn't appear to be support for bending the theoretical definition of undef to the practical constraints of real-life floating-point.

So our options are:
1. Add a constant fold for this exact case: fsub -0.0, undef --> undef
2. Ignore the diffs caused by removing this transform.

I'd lean toward #1 (I can limit D74713 to that case as the start of that effort).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73978