[PATCH] D135619: [RISCV][LegalizeTypes][TargetLowering] Add a target hook to limit alignment of scalable vectors in SelectionDAG::getReducedAlign.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2304
 
+  // Limit scalable vector alignment if requested by the target.
+  if (VT.isScalableVector())
----------------
frasercrmck wrote:
> reames wrote:
> > Looking at this code structure, it really seems like what you're doing here is indirectly implementing a lower alignment requirement. 
> > 
> > Digging through the DataLayout code, I'm struggling to tell where our alignment for scalable vectors is actually coming from.  It doesn't appear to be the default data layout string from RISCVTargetMachine, and if we hit the default, we'd seem to be getting values less than 16.  How'd we get an perf alignment of greater than 16 to start with?  Is this maybe a high LMUL case hitting the fallthrough logic in DataLayout::getAlignment?  That's the only thing I can think of.
> > 
> > If so, I think this is fixing the issue in the wrong place.  I think we need to decide on abi and perf alignments for scalable vector types, and then ensure that DataLayout returns those values.  If I'm reading the code correctly, we probably end up with sane values for LMUL=1 and LMUL=2 types, but get bogus results for LMUL>=4.  
> > 
> > Only once we've decided on what the preferred alignment should be does it make sense to add a cap here.  And even then, it can only cap preferred alignment, not abi alignment.  
> The alignment for scalable vectors comes from the known minimum size. The DataLayout's default settings for vectors mean that this just returns the type size for the alignment. So any vector (fixed or scalable) with a minimum size of > 16 means the stack realignment triggers.
> 
> I had a patch which was intended to make it easier for us to specify vector alignments in the DataLayout without listing all possible min sizes we support (which is quite a few):  D112531.
> 
> As for applying that to RISC-V, I was never sure about using an RVV-enabled DataLayout in non-RVV contexts (as vectors would be scalarized, and we don't want to unnecessarily introduce non-XLEN alignments to vectors. I couldn't see an example of targets which change DataLayouts based on feature flags as RVV is. Maybe if our alignments were always XLEN it would be okay, but legally in RVV we could have a 1-byte alignment for i8 vectors, 2-byte for i16 vectors, etc.
> Looking at this code structure, it really seems like what you're doing here is indirectly implementing a lower alignment requirement. 
> 
> Digging through the DataLayout code, I'm struggling to tell where our alignment for scalable vectors is actually coming from.  It doesn't appear to be the default data layout string from RISCVTargetMachine, and if we hit the default, we'd seem to be getting values less than 16.  How'd we get an perf alignment of greater than 16 to start with?  Is this maybe a high LMUL case hitting the fallthrough logic in DataLayout::getAlignment?  That's the only thing I can think of.
> 
> If so, I think this is fixing the issue in the wrong place.  I think we need to decide on abi and perf alignments for scalable vector types, and then ensure that DataLayout returns those values.  If I'm reading the code correctly, we probably end up with sane values for LMUL=1 and LMUL=2 types, but get bogus results for LMUL>=4.  

I think it is LMUL >= 4 cases that are failing because getKnownMinSize is larger than 16.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:11734
       // Store the argument in a stack slot and pass its address.
       Align StackAlign =
           std::max(getPrefTypeAlign(Outs[i].ArgVT, DAG),
----------------
reames wrote:
> Your change here looks correct to me, and could be separated into it's own patch.  I would recognize the code a bit.  The way I would explain what this is doing is as follows:
> 
> This code is creating a stack slot whose address will be passed to the callee.  The alignment of the stack slot must satisfy the abi alignment for type.  The psABI document appears to be silent on the required abi alignment for a vector type.  Given hardware requirements, I believe having the abi alignment follow the alignment of the element type is reasonable.  The largest such abi alignment would be 8.  
> 
> We would prefer to use a larger alignment if profitable - i.e. the perf alignment - but can not use an alignment larger than the stack alignment without additional stack alignment (which is both unimplemented, and likely unprofitable.)  Thus we chose some value greater than 8 and less than equal to the stack alignment.  
> 
> 
> Does the above match your understanding of what's going on here?
> Your change here looks correct to me, and could be separated into it's own patch.  I would recognize the code a bit.  The way I would explain what this is doing is as follows:
> 
> This code is creating a stack slot whose address will be passed to the callee.  The alignment of the stack slot must satisfy the abi alignment for type.  The psABI document appears to be silent on the required abi alignment for a vector type.  Given hardware requirements, I believe having the abi alignment follow the alignment of the element type is reasonable.  The largest such abi alignment would be 8.  
> 
> We would prefer to use a larger alignment if profitable - i.e. the perf alignment - but can not use an alignment larger than the stack alignment without additional stack alignment (which is both unimplemented, and likely unprofitable.)  Thus we chose some value greater than 8 and less than equal to the stack alignment.  
> 
> 
> Does the above match your understanding of what's going on here?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135619