[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 inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2467
 
-  assert(Instance.Lane > 0
+  assert((Instance.Lane > 0 || Instance.isNonConstLane())
              ? !Cost->isUniformAfterVectorization(cast<Instruction>(V), VF)
----------------
should this be implied by Instance.Lane > 0? (implemented with operator overload, `Instance > 0`)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2488-2494
+  if (Instance.isNonConstLane()) {
+    assert(Instance.Kind == VPIteration::LK_ScalableLast &&
+           "Cannot handle other non-constant lane types");
+    Lane = Builder.CreateSub(getRuntimeVF(Builder, Builder.getInt32Ty(), VF),
+                             Builder.getInt32(1 + Instance.Lane));
+  } else
+    Lane = Builder.getInt32(Instance.Lane);
----------------
nit:

  switch (Instance.Kind) {
  case VPIteration::LK_First:
    Lane = Builder.getInt32(Instance.Lane);
    break;
  case VPIteration::LK_ScalableLast:
    Lane = Builder.CreateSub(...);
    break;
  }

(without default). That will give a compile-time warning when a new kind is added.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4428
+      else
+        Lane = VF.getKnownMinValue() - 1;
+    }
----------------
Should Lane not be set to VF.getKnownMinValue() - 1 for VF.isScalable() as well?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:101-108
+  /// There are cases where we wish to extract a particular lane of a vector,
+  /// but don't know the exact value at compile time. For example, we may wish
+  /// to extract the last lane of a scalable vector and this requires using a
+  /// runtime calculation. The enum below describes how the 'Lane' is
+  /// interpreted. The default is LK_First, which means the lane offset from
+  /// the start of the vector. If you want to extract a lane at an offset from
+  /// the end, i.e. LastLane - Lane, you can set the lane 'Kind' to
----------------
How about:

  /// LaneKind describes how to interpret Lane.
  /// For LK_First, Lane is the index into the first N elements of a fixed-vector <N x <etltty>> or a scalable vector <vscale x N x <eltty>>.
  /// For LK_ScalableLast, Lane is the index into the last N elements of a scalable vector <vscale x N x  <eltty>>


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:180
+  /// Returns the number of lanes that we are able to cache.
+  unsigned getNumCachedLanes() const { return 2 * VF.getKnownMinValue(); }
+
----------------
This only needs to be `2 * VF.getKnownMinValue()` if VF is scalable.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:187
+      return VF.getKnownMinValue() + Instance.Lane;
+    default:
+      assert(Instance.Kind == VPIteration::LK_First);
----------------
use LK_First and drop the default, so that you get a compile-time warning if a new kind is added to the enum.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:205
            "ScalarParts has wrong dimensions.");
-    return Entry[Instance.Part][Instance.Lane] != nullptr;
+    unsigned CacheIdx = mapInstanceLaneToIndex(Instance);
+    return Entry[Instance.Part][CacheIdx] != nullptr;
----------------
Can you make this a method to VPIteration, e.g. `getIndex()`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:102
+  enum NonConstantLanes {
+    LAST_LANE = 0xFFFFFFFF,
+    FIRST_NON_CONST_LANE = LAST_LANE,
----------------
david-arm wrote:
> sdesmalen wrote:
> > For scalable vectors, it probably only ever makes sense to capture any of the following lanes:
> > * The first N lanes from <vscale x N x <eltty>>
> > * The last N lanes from <vscale x N x <eltty>>
> > 
> > I'm not sure if the loop-vectorizer would currently ever need more than just the first/last lane, but I could imagine for interleaving it may want to extract the second/third/fourth-last value from the vector.
> > 
> > Perhaps you can represent this with:
> >   unsigned LaneIdx;
> >   enum {
> >     LK_Fixed,
> >     LK_ScalableFirst,
> >     LK_ScalableLast,
> >   } LaneKind;
> > ?
> I'm happy with the idea of adding an extra member to VPInstance that contains an enum and is probably nicer than what I have now! I'm not sure if we need a LK_ScalableFirst though as this is always known at compile time to be 0 I think - perhaps we just need a LK_First and a LK_ScalableLast? Also, are suggesting that the enum describes how to use Lane, i.e.
> 
> if (StartFromFirst)
>   Index = Lane
> else if (StartFromLast)
>   Index = NumElts - 1 - Lane?
> 
> or with LK_ScalableLast do you literally mean the last lane of the vector?
I indeed meant that LaneKind indexes the first or last 'chunk' in the vector, e.g. `v0, v1` for the first, and `vN-2, vN-1` for the last in:

  <vscale x 2 x i32> <=> <elt0, elt1 |  elt2, elt3 | ... | eltN-2, eltN-1>



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

https://reviews.llvm.org/D95139