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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally 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
----------------
spatel wrote:
> cameron.mcinally wrote:
> > spatel wrote:
> > > 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?
> > Sure, or I can do it. I have some time to work on LLVM specific projects right now.
> > 
> > The problematic case is X=+/-NaN. This only applies to FNEG(X) -> FSUB(-0.0, X) transforms, since IEEE-754 *does not* specify the sign-bit of a NaN result for FSUB(-0.0, +/-NaN). IEEE-754 *does* specify the sign-bit for a FNEG(+/-NaN).
> > 
> > That said, IIRC, some architectures make mistakes in practice with FSUB where X=+/-0.0. But, that *is* well defined by IEEE-754. I can brush up on this case if you'd like more detail.
> > 
> > I'll also add that FSUB(-0.0, X) -> FNEG(X) may not be safe for the constrained intrinsics when rounding mode is in effect. I haven't studied that close enough yet, but I've seen enough verbiage in IEEE-754 to know I should be worried about it. 
> > 
> Great - please go ahead with additional examples/docs. IMO, we can always use more of that.
> 
> Anything left to do with this patch?
> Great - please go ahead with additional examples/docs. IMO, we can always use more of that.

No problem. On my TODO list.


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

https://reviews.llvm.org/D61516