[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 2 inline comments as done.
cameron.mcinally added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12358-12360
+      DenormalMode DenormMode = DAG.getDenormalMode(VT);
+      if (DenormMode == DenormalMode::Invalid ||
+          DenormMode == DenormalMode::IEEE) {
----------------
arsenm wrote:
> This will need updating for the splitting the input and output patch I just committed 
Ok, thanks. Will wait for the builds to go green and then update.


================
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();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73978