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

[David Sherwood via Phabricator via llvm-commits]

david-arm 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)
----------------
sdesmalen wrote:
> 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.
I'm not sure about having enum specific getAsXX functions to be honest. Having to add a new function for any new types people want to add in the future seems a bit inflexible. However, I'll try to find a way of doing something better here if possible. I do take your point about making member variables private, but that will require another NFC patch to change `struct` to `class` and adding get/set variables first. It won't be a small change. :)


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

https://reviews.llvm.org/D95139