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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:987
 
+  // Transform PTEST_ANY(X, X) -> PTEST_ANY(PG, X).
+  // Later optimizations may rewrite sequence to use the flag-setting variant
----------------
Perhaps `PTEST_ANY(X=OP(PG,...), X) -> PTEST_ANY(PG, X))` to show where `PG` comes from.


================
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))) {
----------------
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`.


================
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
----------------
Is the `zext` necessary? Same goes for the other tests.


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

https://reviews.llvm.org/D134946