[PATCH] D139068: [AArch64][SVE] Allow to lower WHILEop operations with constant operands to PTRUE

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4327
+  APInt NumActiveElems;
+  unsigned ElementSize = 128 / Op.getValueType().getVectorMinNumElements();
+  if (IsLess) {
----------------
nit: Move `ElementSize` closer to its use.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4329
+  if (IsLess) {
+    NumActiveElems = IsSigned ? Op.getConstantOperandAPInt(2).ssub_ov(
+                                    Op.getConstantOperandAPInt(1), Overflow)
----------------
nit: Can you move these out into separate variables, e.g. X and Y?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4339
+  }
+  if (Overflow)
+    return None;
----------------
Should this Overflow test be moved below the `if (IsOpEqualOrSame) { ... }` condition?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4863-4871
     if (isa<ConstantSDNode>(Op.getOperand(1)) &&
         isa<ConstantSDNode>(Op.getOperand(2))) {
       unsigned MinSVEVectorSize =
           std::max(Subtarget->getMinSVEVectorSizeInBits(), 128u);
-      unsigned ElementSize = 128 / Op.getValueType().getVectorMinNumElements();
-      unsigned NumActiveElems =
-          Op.getConstantOperandVal(2) - Op.getConstantOperandVal(1);
       Optional<unsigned> PredPattern =
-          getSVEPredPatternFromNumElements(NumActiveElems);
-      if ((PredPattern != None) &&
-          NumActiveElems <= (MinSVEVectorSize / ElementSize))
+          getSVEPTruePredNumElements(IntNo, Op, MinSVEVectorSize);
+      if (PredPattern != None)
----------------
Is it worth just moving all the behaviour into that function, and passing IsSigned, IsLess and IsEqual as parameters, such that you get:

  case Intrinsic::aarch64_sve_whilelo:
    return optimizeWhile(Op.getOperand(1), Op.getOperand(2), /*IsSigned=*/false, /*IsLess=*/true, /*IsEqual=*/false);
  case Intrinsic::aarch64_sve_whilels:
    return optimizeWhile(Op.getOperand(1), Op.getOperand(2), /*IsSigned=*/false, /*IsLess=*/true, /*IsEqual=*/true);
  case Intrinsic::aarch64_sve_whilelt:
    return optimizeWhile(Op.getOperand(1), Op.getOperand(2), /*IsSigned=*/true, /*IsLess=*/true, /*IsEqual=*/false);
  case Intrinsic::aarch64_sve_whilele:
    return optimizeWhile(Op.getOperand(1), Op.getOperand(2), /*IsSigned=*/true, /*IsLess=*/true, /*IsEqual=*/true);
  ...



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

https://reviews.llvm.org/D139068