[PATCH] D61516: [SelectionDAG] fold 'fneg undef' to undef

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked an inline comment as done.
spatel added inline comments.


================
Comment at: llvm/test/CodeGen/X86/vec_fneg.ll:52
-; X64-SSE2-NEXT:    retq
+; X64-SSE-LABEL: scalar_fsub_neg0_undef:
+; X64-SSE:       # %bb.0:
+; X64-SSE-NEXT:    retq
   %r = fsub float -0.0, undef
   ret float %r
----------------
cameron.mcinally wrote:
> arsenm wrote:
> > spatel wrote:
> > > arsenm wrote:
> > > > spatel wrote:
> > > > > arsenm wrote:
> > > > > > It seems problematic that the DAG lowering is still producing an fneg for this
> > > > > I'm not following. Is there some place before/after getNode() that we should also fix?
> > > > I mean I don't expect fsub -0, x to be equivalent to fneg x anymore. This test shouldn't be hitting this path, and should be using the fneg instruction?
> > > Ah, I see. So we need to get our patches in order. 
> > > I don't think we're ready to pull the plug on SDAG converting 'fsub -0.0, x' to fneg yet because we don't have that canonicalization in IR yet, but let me know if I'm wrong.
> > > Either way, I should have added tests with fneg in the IR, so we don't lose coverage when we do flip that switch.
> > Yes, I assumed this compatibility hack was still in here somewhere, but we need to start adding tests for the pure fneg
> > I mean I don't expect fsub -0, x to be equivalent to fneg x anymore. This test shouldn't be hitting this path, and should be using the fneg instruction?
> 
> That's actually ok to do. What isn't ok is FNEG(X)->FSUB(-0.0, X). FNEG(X) has clearly defined outputs for some edges cases, e.g. NaNs. FSUB(-0.0, X) does not.  
I suspect the subtlety of the NaN behavior diff is not known/forgotten by most people. Should I add a blurb to the LangRef and/or SDAG node code comments about that?


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

https://reviews.llvm.org/D61516