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

Sanjay Patel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 8 09:11:30 PDT 2022


spatel added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:16729
+
+  SDNodeFlags SelFlags = Op1->getFlags();
+  if (!isIdentitySplat(Op1.getOperand(2), SelFlags.hasNoSignedZeros()))
----------------
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.


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

https://reviews.llvm.org/D127275



More information about the llvm-commits mailing list