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

[David Sherwood via Phabricator via llvm-commits]

david-arm marked 16 inline comments as done.
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:
> 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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:117
+
+  bool isNonConstLane() const { return Kind != LK_First; }
 };
----------------
fhahn wrote:
> Can this have a better name, e.g. in line with the enum value or the Boolean variable name, if you change it?.
Hi @fhahn It's hard to come up with a much better name - how about "isKnownLane"? This terminology is used throughout the codebase to mean something that is known at compile time, e.g. ElementCount::getKnownMinValue()


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:187
+      return VF.getKnownMinValue() + Instance.Lane;
+    default:
+      assert(Instance.Kind == VPIteration::LK_First);
----------------
sdesmalen wrote:
> use LK_First and drop the default, so that you get a compile-time warning if a new kind is added to the enum.
Sadly this way I also get a compile-time warning about ending the function without returning a value, which means I still have to add a default here.


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

https://reviews.llvm.org/D95139