[PATCH] D134946: [AArch64][SVE] Add instcombine for PTEST_ANY(X, X) -> PTEST_ANY(PG, X)

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked an inline comment as done and an inline comment as not done.
c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:1335
+    auto *PredMask = MRI->getUniqueVRegDef(Pred->getOperand(1).getReg());
+    if (Mask != PredMask && (Mask != Pred))
+      return false;
----------------
paulwalker-arm wrote:
> bsmith wrote:
> > Extra parenthesis
> Are you sure this is universally true?  The `s` variants do an implicitly `ptest` using the original instruction's general predicate operand:
> ```
> brkb p0.b, p1/z, p2.b; ptest p1, p0.b ==> brkbs p0.b, p1/z, p2.b
> ```
> If you perform this transformation when the general predicates of both instructions differ the implicit ptest will calculate a result by considering a different set of lanes.
> 
> Whilst the logic might be safe for your `ptest.any` case because it's essentially an OR reduction (and thus safe when pg & data == data), the same is unlikely for things like cc.first and cc.none where it's critical to test the exact lanes requested.
> 
> Assuming you agree and I've not misunderstood you probably want to do something specific for `ptest.any`, which is best done during isel lowering.
> 
> 
> Are you sure this is universally true?  The `s` variants do an implicitly `ptest` using the original instruction's general predicate operand:
> ```
> brkb p0.b, p1/z, p2.b; ptest p1, p0.b ==> brkbs p0.b, p1/z, p2.b
> ```
> If you perform this transformation when the general predicates of both instructions differ the implicit ptest will calculate a result by considering a different set of lanes.
> 
> Whilst the logic might be safe for your `ptest.any` case because it's essentially an OR reduction (and thus safe when pg & data == data), the same is unlikely for things like cc.first and cc.none where it's critical to test the exact lanes requested.
> 
> Assuming you agree and I've not misunderstood you probably want to do something specific for `ptest.any`, which is best done during isel lowering.
> 
> 

You're right this isn't safe for first/last, I missed that. I've updated the patch as suggested and moved the transform to inst combine. Cheers!


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

https://reviews.llvm.org/D134946