[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:2463
 
-  assert(Instance.Lane > 0
+  assert((Instance.Lane > 0 || !Instance.isKnownLane())
              ? !Cost->isUniformAfterVectorization(cast<Instruction>(V), VF)
----------------
nit:

  assert((!Instance.isFirstLane() ||
          Cost->isUniformAfterVectorization(cast<Instruction>(V), VF))
            && "Uniform values only have lane zero");
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4427
+        // In this case 'Lane' refers to the lane offset from the last lane
+        Kind = VPIteration::LaneKind::LK_ScalableLast;
+      else
----------------
Can you restructure this so that either:
* Lane/Kind are explicitly set for both Fixed and Scalable case. Or alternatively:
* Both Lane and Kind are initialized for Fixed, and only updated for scalable.


================
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)
----------------
david-arm wrote:
> sdesmalen wrote:
> > should this be implied by Instance.Lane > 0? (implemented with operator overload, `Instance > 0`)
> I thought that adding an operator overload to struct VPIteration seemed a bit unnecessary for just this one case. The way I've defined LK_ScalableFirst to work means that  the pair (Instance.Lane=0,Instance.Kind=LK_ScalableFirst) actually refers to the last element of the vector, i.e. LastLane - Instance.Lane.
Okay, fair enough on not overloading the operator.

I'm a bit concerned about exposing Lane directly though, because this now means you'll need to check Instance.isKnownLane() to know whether this concerns the first lane, or the last. Any code that forgets to check this, may make the wrong assumptions.

I'd suggest making Lane private, and adding interfaces such as:
* getAsFirst(int Lane)
* getAsScalableLast(int Lane)
which asserts that the lane starts at the beginning (getFromFirst) of from the back of a scalable vector (getFromScalableLast).
It would also remove the need for additional asserts you added to check if the VPIteration Instance is a known lane.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4428
+      else
+        Lane = VF.getKnownMinValue() - 1;
+    }
----------------
david-arm wrote:
> sdesmalen wrote:
> > Should Lane not be set to VF.getKnownMinValue() - 1 for VF.isScalable() as well?
> Not in the way I've defined it, i.e. LastLane - Lane. This is a bit similar to how the offset is defined for Cullen's vector.splice intrinsic. A Lane of 0 is the same as LastLane - this does make the calculation of the runtime lane a bit easier.
What I don't like about that definition is that the representation in the LoopVectorizer is entirely different for scalable and fixed-width vectors. `{0, First}` and `{0, LastScalable}` mean the first and last element, respectively. `{3, First}` means the last for `<4 x i32>` and `{3, LastScalable}` means 4th last element (for `<vscale x 4 x i32>`). I think this is confusing and error prone. If First/ScalableLast would mean the first and last scalable "chunk" of a vector, with the index being the index as normal, then the last iteration would be described as `{3, First}` for `<4 x i32>` and `{3, ScalableLast}`  for `<vscale x 4 x i32>`, and so the representation in the LoopVectorizer is more or less the same. Code-generating it is slightly different of course, but there's probably only a single place where that has to happen.


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

https://reviews.llvm.org/D95139