[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:
> 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?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73978