[PATCH] D120152: [AArch64][SVE] Match VLS all-1's masks to PTRUE

[David Sherwood via Phabricator via llvm-commits]

david-arm added a subscriber: craig.topper.
david-arm added a comment.

Hi @craig.topper, I really like what you're trying to do in this patch and the codegen indeed looks a lot better! I just had some suggestions about a possibly simpler, and more comprehensive approach that might give us more overall benefit.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17030
+    if (isAllActivePredicate(DAG, Pred) && LHS.getOpcode() == ISD::AND &&
+        isPow2Splat(LHS.getOperand(1), SplatVal, Negated) && SplatVal == 1) {
+      // We found a vXi1 truncate. Now check if we're truncating a fixed width
----------------
I think you also need to check for `&& !Negated` here. Alternatively, I think you could just do:

  APInt SplatVal;
  if (isAllActivePredicate(DAG, Pred) && LHS.getOpcode() == ISD::AND &&
      ISD::isConstantSplatVector(LHS.getOperand(1), SplatVal) && SplatVal == 1) {

The only additional value that `isPow2Splat` adds here is that it also checks for AArch64ISD::DUP nodes, but I imagine at the point we're doing the DAG combines here we haven't generated an AArch64 ISD node yet?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17036
+      bool TruncNegated;
+      if (Trunc.getOpcode() == ISD::INSERT_SUBVECTOR &&
+          Trunc.getOperand(0).isUndef() &&
----------------
It feels like we should have a more basic DAG combine here, i.e. something in either `DAGCombiner::visitINSERT_SUBVECTOR` or in `performInsertSubvectorCombine`, that basically combines

  <vscale x M x iXY> insert_subvector <vscale x M x iXY> undef, <N x iXY> <splat of iXY A>

into

  <vscale x M x iXY> <splat of iXY A>

when we know that vscale x M == N.

If you implement such a DAG combine it might benefit other parts of the code too? It would then mean here you should only have to check if `Trunc` is a splat of 1, which may also catch more cases.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17038
+          Trunc.getOperand(0).isUndef() &&
+          isPow2Splat(Trunc.getOperand(1), TruncSplatVal, TruncNegated) &&
+          TruncSplatVal == 1) {
----------------
Again, here I think you need to check `&& !TruncNegated`


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

https://reviews.llvm.org/D120152