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

[Cullen Rhodes via Phabricator via llvm-commits]

c-rhodes marked 3 inline comments as done.
c-rhodes added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:993
+      ((Op1->getIntrinsicID() == Intrinsic::aarch64_sve_brkb_z) ||
+       (Op1->getIntrinsicID() == Intrinsic::aarch64_sve_brkn_z) ||
+       (Op1->getIntrinsicID() == Intrinsic::aarch64_sve_rdffr_z))) {
----------------
paulwalker-arm wrote:
> Looking at the instruction pseudo code for `brkns` it works differently than the other instructions that set the flags in that its implicit predicate is an all active one.  This likely means there's a bug in `AArch64InstrInfo::optimizePTestInstr`.
> Looking at the instruction pseudo code for `brkns` it works differently than the other instructions that set the flags in that its implicit predicate is an all active one.  This likely means there's a bug in `AArch64InstrInfo::optimizePTestInstr`.

Good spot! I've removed `brkn` from this patch and will fix the bug in `AArch64InstrInfo::optimizePTestInstr` in a separate patch. I guess the optimization there can still be done as long as the ptest is called with all active mask rather than the mask from the BRKN instruction?


================
Comment at: llvm/test/Transforms/InstCombine/AArch64/sve-intrinsics-ptest.ll:45
+  %2 = call i1 @llvm.aarch64.sve.ptest.any.nxv16i1(<vscale x 16 x i1> %1, <vscale x 16 x i1> %1)
+  %conv = zext i1 %2 to i32
+  ret i32 %conv
----------------
paulwalker-arm wrote:
> Is the `zext` necessary? Same goes for the other tests.
> Is the `zext` necessary? Same goes for the other tests.

Hangover from previous revision, fixed.


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

https://reviews.llvm.org/D134946