[PATCH] D127275: [MVE] Fold fadd(select(..., +0.0)) into a predicated fadd

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 2 inline comments as done.
david-arm added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:16729
+
+  SDNodeFlags SelFlags = Op1->getFlags();
+  if (!isIdentitySplat(Op1.getOperand(2), SelFlags.hasNoSignedZeros()))
----------------
spatel wrote:
> david-arm wrote:
> > spatel wrote:
> > > This is not strictly correct. If the 'fadd' doesn't have 'nsz', then the transform results in an 'nsz' value where the original sequence did not.
> > > 
> > > The transform is safe as long as the 'fadd' has 'nsz', so that's the flag we should use to enable the transform.
> > > 
> > > We can propagate the fadd's 'nsz' to the new 'select' even if the original 'select' was not 'nsz' itself. 
> > > It's confusing, but you can run examples with Alive2 to see what's valid:
> > > https://alive2.llvm.org/ce/z/Z-cuhQ
> > Doesn't this contradict https://reviews.llvm.org/D126774 then? In my original instcombine patch I think it was suggested that we should add nsz to the select instruction being generated by the vectoriser, whereas from this comment it sounds like we don't need to and can just rely upon the nsz flag on the IR fadd?
> No, the transforms are not strictly reversible/bidirectional:
> https://alive2.llvm.org/ce/z/G42d8e
> 
> In both cases, I think we need 'nsz' on the **final** value in the pattern to enable the transform safely.
> It is possible that we've created some unintended FMF propagation though when the flags are different on each value. 
> 
> The semantics are fuzzy and having different flags on values within a function seems highly unlikely, but we should probably add more tests to cover those possibilities.
OK thanks for explaining and looking into this in detail! I guess differences in flags are much more likely to occur with inlining, especially as part of LTO where different translation units may be compiled differently.


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

https://reviews.llvm.org/D127275