[PATCH] D118147: [AArch64][SVE] Folds VSELECT if the predicate is all active.

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15112
+          getNumElementsFromSVEPredPattern(N.getConstantOperandVal(0));
+      return PatNumElts == NumElts * VScale;
+    }
----------------
nit: Maybe this is just personal preference, but I think these types of comparisons are a bit easier to read with (), i.e. `PatNumElts == (NumElts * VScale)`.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15112
+          getNumElementsFromSVEPredPattern(N.getConstantOperandVal(0));
+      return PatNumElts == NumElts * VScale;
+    }
----------------
david-arm wrote:
> nit: Maybe this is just personal preference, but I think these types of comparisons are a bit easier to read with (), i.e. `PatNumElts == (NumElts * VScale)`.
It looks like we can also support the case where PatNumElts > (NumElts * VScale) as well, right?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16803
 
+  if (isAllActivePredicate(DAG, N0))
+    return N->getOperand(1);
----------------
This is purely an observation, but we can also do something similar for all-false predicates by selecting y from (x, y) inputs. I imagine this is much less commonly seen in practice though ...


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:16809
   // supported types.
   SDValue SetCC = N->getOperand(0);
   if (SetCC.getOpcode() == ISD::SETCC &&
----------------
This is not something introduced by your patch (but if you choose to fix it then great!), but we call `N->getOperand(0)` twice and have two different variables for it - `N0` and `SetCC`. It's a bit confusing!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118147