[PATCH] D129282: [AArch64][SVE] Ensure PTEST operands have type nxv16i1

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16192
+    case AArch64CC::NONE_ACTIVE:
+      if (isZeroingInactiveLanes(Op) || isZeroingInactiveLanes(Pg))
+        Pg = DAG.getNode(AArch64ISD::REINTERPRET_CAST, DL, MVT::nxv16i1, Pg);
----------------
For the same reason as below I think this can be just `if (isZeroingInactiveLanes(Op))`


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16199-16202
+      if (isZeroingInactiveLanes(Pg))
+        Pg = DAG.getNode(AArch64ISD::REINTERPRET_CAST, DL, MVT::nxv16i1, Pg);
+      else
+        Pg = getSVEPredicateBitCast(MVT::nxv16i1, Pg, DAG);
----------------
Isn't this equivalent to just `Pg = getSVEPredicateBitCast(MVT::nxv16i1, Pg, DAG);`? because `getSVEPredicateBitCast` already has the `isZeroingInactiveLanes` logic.  Which means these two cases can be removed as the default case should just work?


================
Comment at: llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td:2134-2135
 
   def : Pat<(AArch64ptest (nxv16i1 PPR:$pg), (nxv16i1 PPR:$src)),
             (PTEST_PP PPR:$pg, PPR:$src)>;
 
----------------
Now that we'll only ever have a single pattern, can you do this as part of `PTEST_PP`'s definition?


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

https://reviews.llvm.org/D129282