[PATCH] D95350: [SVE][LoopVectorize] Add gather/scatter support for SVE

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll:12
+entry:
+  %cmp10 = icmp sgt i64 %n, 0
+  br i1 %cmp10, label %for.body.preheader, label %for.cond.cleanup
----------------
fhahn wrote:
> This shouldn't be necessary, LV will generate a minimum iteration check anyways.
Sure, I simply adapted these tests by dumping out the module before the vectoriser, i.e. I didn't hand craft the tests. :) I can remove them no problem.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll:15
+
+for.body.preheader:                               ; preds = %entry
+  br label %for.body
----------------
fhahn wrote:
> are those blocks needed?
Probably not. Again I didn't write these IR tests by hand - I dumped out the module before the vectoriser so that explains why these blocks are here. I can try to clean them up.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-gather-scatter.ll:28
+  %0 = load i32, i32* %arrayidx, align 4
+  %idxprom2 = sext i32 %0 to i64
+  %arrayidx3 = getelementptr inbounds float, float* %a, i64 %idxprom2
----------------
fhahn wrote:
> Is this necessary? Can we instead just load from a `i64*` or use `i32` in the GEP? Same for the other tests.
I deliberately left this in as the sext exposes a different code path in the vectoriser that was broken - see line 7463 in LoopVectorize.cpp. I'd like to have at least one test with a sext of i32 -> i64 so that the fix and test are in the same patch.


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

https://reviews.llvm.org/D95350