[PATCH] D119336: [AArch64][SVE] Avoid multiple PTRUE values for SETCC.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14106-14121
+  // Fold:
+  //    and(
+  //      setcc_merge_zero(pred, ...),
+  //      setcc_merge_zero(pred, ...))
+  // -> setcc_merge_zero(
+  //        setcc_merge_zero(pred, ...),
+  //        ...)
----------------
I thought we have an isel pattern to capture this.  Is it the case you need to catch it early? If so then I think you should also remove the isel pattern to prove it's redundant.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17096
+// fixed-width vector -> scalable vector.
+static bool isConvertToScalableVector(SDValue V) {
+  return V.getOpcode() == ISD::INSERT_SUBVECTOR && V.getOperand(0).isUndef() &&
----------------
This also needs to test `V.getValueType().isScalableVector()` or have an equivalent assert if you never expect it to be called when such is not true.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17124
+  // when the predicate is equivalent.
+  EVT FixedLengthVT;
+  if (isConvertToScalableVector(LHS) && isAllActivePredicate(DAG, Pred)) {
----------------
This looks misplaced?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17128-17129
+    SDLoc DL(N);
+    SDValue FixedLengthPred =
+        getPredicateForFixedLengthVector(DAG, DL, FixedLengthVT);
+    return DAG.getNode(AArch64ISD::SETCC_MERGE_ZERO, DL, N->getValueType(0),
----------------
This has me wondering if we're in fact missing something else. Do you know where the all active PTRUE is coming from?

You're making the assumption that there exists a fixed length PTRUE, which is probably true but still feels like an arbitrary assumption.  For example, when vscale-min==vscale-max we favour all active PTRUEs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119336