[PATCH] D125747: [RISCV] Enable scalable vectorization by default for RVV

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/test/Transforms/LoopVectorize/RISCV/unroll-in-loop-vectorizer.ll:6
 ;
 define void @small_loop(i32* nocapture %inArray, i32 %size) nounwind {
 ; CHECK-LABEL: @small_loop(
----------------
reames wrote:
> craig.topper wrote:
> > reames wrote:
> > > craig.topper wrote:
> > > > reames wrote:
> > > > > reames wrote:
> > > > > > As an aside, I took a look at the assembly for this example.  Th codegen for the vector loop ends up being less than great.  For instance:
> > > > > > * We have a extend trapped in the loop for some reason
> > > > > > * We rerun vsetvli on every iteration (despite it producing a fixed result)
> > > > > > * We have a rem in the vector preheader.  That's rather expensive.  (Well, actually, we end up with a *libcall* because the attributes don't include +m, but I ignored that.)
> > > > > To be fair, the first two issues also exist with fixed length vectorization of the same example.  
> > > > What kind of extend?
> > > zext - it looks easily removable, I'm surprised to see it after O2.  This shouldn't be a hard fix.  It appears in both vector and scalar loops.  
> > Can you share what you're seeing? I see a zero extend in the preheader but not in the loops.
> When running just loop-vectorize and then llc, I do not see the extend.  When replacing -loop-vectorize with -O2 on the same input, I do.  
> 
> ```
> ./opt -mtriple=riscv64 -mattr=+v,+m test/Transforms/LoopVectorize/RISCV/unroll-in-loop-vectorizer.ll -loop-vectorize -scalable-vectorization=on -S -riscv-v-vector-bits-max=512 | ./llc -O3 
> ./opt -mtriple=riscv64 -mattr=+v,+m test/Transforms/LoopVectorize/RISCV/unroll-in-loop-vectorizer.ll -O2 -scalable-vectorization=on -S -riscv-v-vector-bits-max=512 | ./llc -O3 
> ```
> 
> The key bit of IR after -O2 is:
> 
>   %index = phi i32 [ 0, %vector.ph ], [ %index.next, %vector.body ]
>   %13 = zext i32 %index to i64
>   // ... 
>   %index.next = add nuw i32 %index, %12
>   %23 = icmp eq i32 %index.next, %n.vec
>   br i1 %23, label %middle.block, label %vector.body, !llvm.loop !0
> 
> We should be able to widen %index IV in indvars without issue, I'm a bit surprised we don't.  
> 
> Interestingly, if I add Zba, something in codegen manages to fold away the extend now.  We didn't when I'd last looked.
> 
> ```
> ./opt -mtriple=riscv64 -mattr=+v,+m,+zba test/Transforms/LoopVectorize/RISCV/unroll-in-loop-vectorizer.ll -O2 -scalable-vectorization=on -S -riscv-v-vector-bits-max=512 | ./llc -O3
> ```
> 
> To be clear, this is a minor codegen issue at best, and definitely nothing which should block this patch.  
Does opt not automatically infer the DataLayout from the triple? Adding a proper DataLayout seems to make the zext go away.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125747