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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:2304
 
+  // Limit scalable vector alignment if requested by the target.
+  if (VT.isScalableVector())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135619