[PATCH] D81267: [LV] Enable the LoopVectorizer to create pointer inductions

[Anna Welker via Phabricator via llvm-commits]

anwel added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/pointer-induction.ll:4
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
fhahn wrote:
> dmgreen wrote:
> > I think if you use x86 as a target (and needs it for the costing), the test needs to go into test/Transforms/LoopVectorize/X86 in case the target is not compiled in.
> It looks like the options above actually force vectorization with a certain factor. In that case, it Is probably best to remove the triple.
> 
> I'd also consider just checking the loop-vectorize output (without -dce -instcombine), if it is not too messy, as it makes the test more prone to break when something changes in instcombine. Also, it might be possible to only specifically check the IR related to the generated induction, rather than autogenerating the checks, which include a lot of relatively irrelevant stuff.
I thought it did need the target information to behave in the right way, but apparently I was mistaken - so no relocation necessary, I removed the target.


================
Comment at: llvm/test/Transforms/LoopVectorize/pointer-induction.ll:4
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
anwel wrote:
> fhahn wrote:
> > dmgreen wrote:
> > > I think if you use x86 as a target (and needs it for the costing), the test needs to go into test/Transforms/LoopVectorize/X86 in case the target is not compiled in.
> > It looks like the options above actually force vectorization with a certain factor. In that case, it Is probably best to remove the triple.
> > 
> > I'd also consider just checking the loop-vectorize output (without -dce -instcombine), if it is not too messy, as it makes the test more prone to break when something changes in instcombine. Also, it might be possible to only specifically check the IR related to the generated induction, rather than autogenerating the checks, which include a lot of relatively irrelevant stuff.
> I thought it did need the target information to behave in the right way, but apparently I was mistaken - so no relocation necessary, I removed the target.
Thanks for the feedback, I don't have much experience writing opt tests so your advice is very welcome.

I have removed the triple and the meta data, after checking that we don't need them, and reduced the checks to `vector.ph`, `vector.body` and the loop latch that changes the induction variable.


================
Comment at: llvm/test/Transforms/LoopVectorize/pointer-induction.ll:7
+; Function Attrs: nofree norecurse nounwind
+define void @a(i8* readnone %b) local_unnamed_addr #0 {
+; CHECK-LABEL: @a(
----------------
dmgreen wrote:
> Also some of this might be able to be cleaned up, like the local_unnamed_addr, the metadata and all/most(?) of the attributes.
Should be a lot cleaner now.


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

https://reviews.llvm.org/D81267