[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 a comment.

Hi @fhahn thanks for the review!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:224
+  assert(!Instance.isNonConstLane() &&
+         "Non-constant lanes need handling with callbacks!");
+
----------------
fhahn wrote:
> can you add support here as well? The callback is going away soon, so we also need to support the non-callback version.
OK, the reason I didn't add this originally is because I cannot test this code path in my patch. I thought it might be bad practice to add code without testing it, but I'm happy to add support here if you want. I guess we do have a test for it, so when the callback is removed if there is a bug it will break the test.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:110
+  enum LaneKind {
+    LK_First,
+    LK_ScalableLast,
----------------
fhahn wrote:
> are you planning on adding more kinds here? otherwise this can just be a boolean? Or make this an `enum class`?
I don't have plans to add other kinds here at the moment - I chose an enum here to make it extensible should people wish to add other kinds in future and based on earlier reviewer comments. I'm happy to change it to `enum class` or use a boolean - @sdesmalen don't know if you have a preference here?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/neon-extract-last-veclane.ll:3
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
----------------
fhahn wrote:
> This test does not need to be neon specific, right? extracting the last lane for fixed vectors should be tested fairly well already, so not sure if the test is needed at all?
Similar to what I mentioned in the commit message, I deliberately added a llvm_unreachable() in the code that I've changed and I found no explicit tests for this at all. There is some limited coverage, but accidentally so in cases where the test was actually trying to test something else. How about I move this to a generic place?


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-extract-last-veclane.ll:25
+
+for.cond.for.cond.cleanup_crit_edge:              ; preds = %for.body
+  %mul.lcssa = phi i32 [ %mul, %for.body ]
----------------
fhahn wrote:
> nit: the names of the blocks could be improved.
OK, to be honest I don't really know what they should be called. :) This is the name that LLVM generates. How about `for.cond.pre-cleanup`?


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

https://reviews.llvm.org/D95139