[PATCH] D95139: [SVE][LoopVectorize] Add support for extracting the last lane of a scalable vector

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for the changes @david-arm, I think this is moving in the right direction.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2486
 
-  assert(Instance.Lane > 0
+  assert(!Instance.isFirstLane()
              ? !Cost->isUniformAfterVectorization(cast<Instruction>(V), VF)
----------------
nit: Can you maybe write this as `Instance.Lane.isFirst()`? Personally I find the difference between isFirstLane and isFirstIteration a bit confusing. (same for other places)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4449-4455
+      LaneOffset = VF.getKnownMinValue() - 1;
+      if (VF.isScalable())
+        // In this case 'LaneOffset' refers to the offset from the start of the
+        // last subvector with VF.getKnownMinValue() elements.
+        Kind = VPLane::Kind::ScalableLast;
+      else
+        Kind = VPLane::Kind::First;
----------------
is it worth creating a `static VPIteration::getLastLaneForVF(ElementCount)` for this?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:70
+    break;
+  default:
+    assert(LaneKind == VPLane::Kind::First);
----------------
don't use default. Cover both cases explicitly, so that if another enum value is added, the compiler will emit a diagnostic this case is not covered.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:98
+  /// Kind describes how to interpret Val.
+  /// For First, Val is the index into the first N elements of a
+  /// fixed-vector <N x <ElTy>> or a scalable vector <vscale x N x <ElTy>>.
----------------
nit: if you put the comments above each enum-value, doxygen generates a nice table with a comment describing what each enum-value means.
See for example `ARMLdStMultipleTiming` in ARMSubtarget.h (and the generated doxygen here: https://llvm.org/doxygen/classllvm_1_1ARMSubtarget.html#ac7324b67d7e3be270177e6590f0bb1e5)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:108
+  /// in [0..VF)
+  unsigned Val;
+
----------------
I'd prefer this to just be named `Lane`, because it is still a lane. The First or ScalableLast tells in which chunk of the vector this lane lives.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:118
+  /// lane can only be calculated at runtime.
+  unsigned getKnownValue() const {
+    assert(LaneKind == Kind::First);
----------------
nit: `getKnownLane`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:125
+  /// runtime.
+  Value *getExpr(IRBuilder<> &Builder, const ElementCount &VF) const;
+
----------------
nit: `getLaneAsRuntimeExpr` ?


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

https://reviews.llvm.org/D95139