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

Wed Jun 8 08:59:47 PDT 2022

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:
> 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?

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

https://reviews.llvm.org/D127275