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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as not done.
cameron.mcinally 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:
> > 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73978