[RISCV][WIP][RFC] Fix stack slot for argument type sizes not a multiple of 64 bits (Bug 49500)

[luismarques]

Summary

This is a fix for: https://bugs.llvm.org/show_bug.cgi?id=49500.

As it stands, this patch is basically just a copy of D97514. Originally I suspected that there might be a better way to solve it, but eventually I realized I had arrived at roughly the same approach. I'm not sure there's much point in using getRegisterTypeForCallingConv and getNumRegistersForCallingConv versus something more direct, but I've left that unchanged.

[luismarques]

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

6196–6197

Is OrigArgVT always going to be equal to Outs[i].ArgVT?

[frasercrmck]

Does this happen to fix D95025? It's certainly in a similar area as my (WIP) fix that I never got round to publishing.

[luismarques]

In D99068#2641053, @frasercrmck wrote:

Does this happen to fix D95025? It's certainly in a similar area as my (WIP) fix that I never got round to publishing.

At first glance it looks like it does.

[frasercrmck]

In D99068#2641065, @luismarques wrote:

At first glance it looks like it does.

Excellent!

My fix was more direct: I went over ArgValue and all of the SDValue PartValue = OutVals[i + 1]; as in the loop later in this block, and summed up all of the required getStoreSizes of their VTs, and took the maximum required alignment of each. Then created a stack temporary of that summed size and max alignment. I wasn't sufficiently happy with it and since I didn't get much indication that this was an urgent fix I didn't put it up for discussion - sorry about that. I can share my patch with you if you're interested.

[luismarques]

In D99068#2641075, @frasercrmck wrote:

My fix was more direct: I went over ArgValue and all of the SDValue PartValue = OutVals[i + 1]; as in the loop later in this block, and summed up all of the required getStoreSizes of their VTs, and took the maximum required alignment of each. Then created a stack temporary of that summed size and max alignment. I wasn't sufficiently happy with it and since I didn't get much indication that this was an urgent fix I didn't put it up for discussion - sorry about that. I can share my patch with you if you're interested.

That sounds like it has the potential for being a better alternative than this one. Please post it for review. Thanks!

[luismarques]

• Precommitted the original test, to better show the difference caused by the patch.
• Updated the existing tests. They seem to more broadly show the issue I noted about the excessive alignment/overly conservative stack size.

[luismarques]

Abandoned in favor of D99087. Thanks @frasercrmck!

[luismarques]

Fixing alignment before truly abandoning, in case it's useful for D97514.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
   SmallVectorImpl<ISD::InputArg> &Ins = CLI.Ins;
   SDValue Chain = CLI.Chain;
   SDValue Callee = CLI.Callee;
   bool &IsTailCall = CLI.IsTailCall;
   CallingConv::ID CallConv = CLI.CallConv;
   bool IsVarArg = CLI.IsVarArg;
   EVT PtrVT = getPointerTy(DAG.getDataLayout());
   MVT XLenVT = Subtarget.getXLenVT();
+  LLVMContext &Ctx = *DAG.getContext();
 
   MachineFunction &MF = DAG.getMachineFunction();
 
   // Analyze the operands of the call, assigning locations to each operand.
   SmallVector<CCValAssign, 16> ArgLocs;
-  CCState ArgCCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
+  CCState ArgCCInfo(CallConv, IsVarArg, MF, ArgLocs, Ctx);
 
   if (CallConv == CallingConv::Fast)
     ArgCCInfo.AnalyzeCallOperands(Outs, CC_RISCV_FastCC);
   else if (CallConv == CallingConv::GHC)
     ArgCCInfo.AnalyzeCallOperands(Outs, CC_RISCV_GHC);
   else
     analyzeOutputArgs(MF, ArgCCInfo, Outs, /*IsRet=*/false, &CLI);
 
for (unsigned i = 0, j = 0, e = ArgLocs.size(); i != e; ++i) {
 
     // IsF64OnRV32DSoftABI && VA.isMemLoc() is handled below in the same way
     // as any other MemLoc.
 
     // Promote the value if needed.
     // For now, only handle fully promoted and indirect arguments.
     if (VA.getLocInfo() == CCValAssign::Indirect) {
       // Store the argument in a stack slot and pass its address.
-      SDValue SpillSlot = DAG.CreateStackTemporary(Outs[i].ArgVT);
+      unsigned ArgIndex = Outs[i].OrigArgIndex;
+      EVT SlotVT;
+      Align SlotAlign = DAG.getEVTAlign(ArgValue.getValueType());
+      SlotAlign = std::max(DAG.getEVTAlign(Outs[i].ArgVT), SlotAlign);
+      if (i + 1 != e && Outs[i + 1].OrigArgIndex == ArgIndex) {
+        // Allocate the full stack space for a promoted (and split) argument.
+        Type *OrigArgType = CLI.Args[Outs[i].OrigArgIndex].Ty;
+        EVT OrigArgVT = getValueType(MF.getDataLayout(), OrigArgType);
+        MVT PartVT =
+            getRegisterTypeForCallingConv(Ctx, CLI.CallConv, OrigArgVT);
+        unsigned N =
+            getNumRegistersForCallingConv(Ctx, CLI.CallConv, OrigArgVT);
+        SlotVT = EVT::getIntegerVT(Ctx, PartVT.getSizeInBits() * N);
+        Type *SlotTy = SlotVT.getTypeForEVT(*DAG.getContext());
+        SlotAlign =
+            std::max(SlotAlign, DAG.getDataLayout().getPrefTypeAlign(SlotTy));
+      } else {
+        SlotVT = Outs[i].ArgVT;
+      }
+      SDValue SpillSlot = DAG.CreateStackTemporary(SlotVT, SlotAlign.value());
       int FI = cast<FrameIndexSDNode>(SpillSlot)->getIndex();
       MemOpChains.push_back(
           DAG.getStore(Chain, DL, ArgValue, SpillSlot,
                        MachinePointerInfo::getFixedStack(MF, FI)));
       // If the original argument was split (e.g. i128), we need
       // to store the required parts of it here (and pass just one address).
       // Vectors may be partly split to registers and partly to the stack, in
       // which case the base address is partly offset and subsequent stores are
       // relative to that.
-      unsigned ArgIndex = Outs[i].OrigArgIndex;
       unsigned ArgPartOffset = Outs[i].PartOffset;
       assert(VA.getValVT().isVector() || ArgPartOffset == 0);
       while (i + 1 != e && Outs[i + 1].OrigArgIndex == ArgIndex) {
         SDValue PartValue = OutVals[i + 1];
         unsigned PartOffset = Outs[i + 1].PartOffset - ArgPartOffset;
         SDValue Address = DAG.getNode(ISD::ADD, DL, PtrVT, SpillSlot,
                                       DAG.getIntPtrConstant(PartOffset, DL));
         MemOpChains.push_back(

llvm/test/CodeGen/RISCV/stack-slot-size.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
 ; RUN:   | FileCheck -check-prefix=RV32I %s
 ; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
 ; RUN:   | FileCheck -check-prefix=RV64I %s
 
 ; When passing a function argument with a size that isn't a multiple of XLEN,
 ; and the argument is split and passed indirectly, we must ensure that the stack
 ; slot size appropriately reflects the total size of the parts the argument is
 ; split into. Otherwise, stack writes can clobber neighboring values.
 
 declare void @callee129(i129)
 declare void @callee160(i160)
 declare void @callee161(i161)
 
-; FIXME: Stack write clobbers the spilled value (on RV64).
 define i32 @caller129() nounwind {
 ; RV32I-LABEL: caller129:
 ; RV32I:       # %bb.0:
 ; RV32I-NEXT:    addi sp, sp, -32
 ; RV32I-NEXT:    sw ra, 28(sp) # 4-byte Folded Spill
 ; RV32I-NEXT:    addi a0, zero, 42
 ; RV32I-NEXT:    sw a0, 24(sp)
 ; RV32I-NEXT:    sw zero, 16(sp)
 ; RV32I-NEXT:    sw zero, 12(sp)
 ; RV32I-NEXT:    sw zero, 8(sp)
 ; RV32I-NEXT:    sw zero, 4(sp)
 ; RV32I-NEXT:    mv a0, sp
 ; RV32I-NEXT:    sw zero, 0(sp)
 ; RV32I-NEXT:    call callee129@plt
 ; RV32I-NEXT:    lw a0, 24(sp)
 ; RV32I-NEXT:    lw ra, 28(sp) # 4-byte Folded Reload
 ; RV32I-NEXT:    addi sp, sp, 32
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: caller129:
 ; RV64I:       # %bb.0:
-; RV64I-NEXT:    addi sp, sp, -32
-; RV64I-NEXT:    sd ra, 24(sp) # 8-byte Folded Spill
+; RV64I-NEXT:    addi sp, sp, -48
+; RV64I-NEXT:    sd ra, 40(sp) # 8-byte Folded Spill
 ; RV64I-NEXT:    addi a0, zero, 42
-; RV64I-NEXT:    sw a0, 20(sp)
+; RV64I-NEXT:    sw a0, 36(sp)
 ; RV64I-NEXT:    sd zero, 16(sp)
 ; RV64I-NEXT:    sd zero, 8(sp)
 ; RV64I-NEXT:    mv a0, sp
 ; RV64I-NEXT:    sd zero, 0(sp)
 ; RV64I-NEXT:    call callee129@plt
-; RV64I-NEXT:    lw a0, 20(sp)
-; RV64I-NEXT:    ld ra, 24(sp) # 8-byte Folded Reload
-; RV64I-NEXT:    addi sp, sp, 32
+; RV64I-NEXT:    lw a0, 36(sp)
+; RV64I-NEXT:    ld ra, 40(sp) # 8-byte Folded Reload
+; RV64I-NEXT:    addi sp, sp, 48
 ; RV64I-NEXT:    ret
   %1 = alloca i32
   store i32 42, i32* %1
   call void @callee129(i129 0)
   %2 = load i32, i32* %1
   ret i32 %2
 }
 
-; FIXME: Stack write clobbers the spilled value (on RV64).
 define i32 @caller160() nounwind {
 ; RV32I-LABEL: caller160:
 ; RV32I:       # %bb.0:
 ; RV32I-NEXT:    addi sp, sp, -32
 ; RV32I-NEXT:    sw ra, 28(sp) # 4-byte Folded Spill
 ; RV32I-NEXT:    addi a0, zero, 42
 ; RV32I-NEXT:    sw a0, 24(sp)
 ; RV32I-NEXT:    sw zero, 16(sp)
 ; RV32I-NEXT:    sw zero, 12(sp)
 ; RV32I-NEXT:    sw zero, 8(sp)
 ; RV32I-NEXT:    sw zero, 4(sp)
 ; RV32I-NEXT:    mv a0, sp
 ; RV32I-NEXT:    sw zero, 0(sp)
 ; RV32I-NEXT:    call callee160@plt
 ; RV32I-NEXT:    lw a0, 24(sp)
 ; RV32I-NEXT:    lw ra, 28(sp) # 4-byte Folded Reload
 ; RV32I-NEXT:    addi sp, sp, 32
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: caller160:
 ; RV64I:       # %bb.0:
-; RV64I-NEXT:    addi sp, sp, -32
-; RV64I-NEXT:    sd ra, 24(sp) # 8-byte Folded Spill
+; RV64I-NEXT:    addi sp, sp, -48
+; RV64I-NEXT:    sd ra, 40(sp) # 8-byte Folded Spill
 ; RV64I-NEXT:    addi a0, zero, 42
-; RV64I-NEXT:    sw a0, 20(sp)
+; RV64I-NEXT:    sw a0, 36(sp)
 ; RV64I-NEXT:    sd zero, 16(sp)
 ; RV64I-NEXT:    sd zero, 8(sp)
 ; RV64I-NEXT:    mv a0, sp
 ; RV64I-NEXT:    sd zero, 0(sp)
 ; RV64I-NEXT:    call callee160@plt
-; RV64I-NEXT:    lw a0, 20(sp)
-; RV64I-NEXT:    ld ra, 24(sp) # 8-byte Folded Reload
-; RV64I-NEXT:    addi sp, sp, 32
+; RV64I-NEXT:    lw a0, 36(sp)
+; RV64I-NEXT:    ld ra, 40(sp) # 8-byte Folded Reload
+; RV64I-NEXT:    addi sp, sp, 48
 ; RV64I-NEXT:    ret
   %1 = alloca i32
   store i32 42, i32* %1
   call void @callee160(i160 0)
   %2 = load i32, i32* %1
   ret i32 %2
 }