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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


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


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:518
           MF.getFunction(), "Frame pointer required, but has been reserved."});
+    // The frame pointer does need to be reserved from register allocation.
+    assert(MF.getRegInfo().isReserved(FPReg) && "FP not reserved");
----------------
Good idea!


================
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),
----------------
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