[mips] LLVM PR/30197 - Tail call incorrectly clobbers arguments

[sdardis]

Summary

The postRA scheduler performs alias analysis to determine if stores and loads
can moved past each other. When a function has more arguments than argument
registers for the calling convention used, excess arguments are spilled onto the
stack. LLVM by default assumes that argument slots are immutable, unless the
function contains a tail call. Without the knowledge of that a function contains
a tail call site, stores and loads to fixed stack slots may be re-ordered
causing the out-going arguments to clobber the incoming arguments before the
incoming arguments are supposed to be dead.

[vkalintiris]

LGTM but make sure that the checks from the new test enforce the correct ordering of loads/stores.

test/CodeGen/Mips/tailcall/tail-call-arguments-clobber.ll

19

We need a CHECK-LABEL in order to avoid sourcing instructions from func3 (similarly for func3).

21–24

I don't think that this set of checks will enforce the ordering of loads/stores (similarly for func3).

[sdardis]

test/CodeGen/Mips/tailcall/tail-call-arguments-clobber.ll

21–24

Are you expecting the loads to come in a specific order? I was testing that the loads comes before the stores, as otherwise the callee/caller argument area gets clobbered. I didn't see the value in checking that 16(sp) comes before 20(sp) for the loads.

The original bug is this:

lui $2, %hi(_gp_disp)
addiu $2, $2, %lo(_gp_disp)
addu  $gp, $2, $25
lw  $2, 16($sp)
sw  $5, 20($sp)  <- swapped with the load beneath
lw  $1, 20($sp)  <- should be before the store
sw  $2, 16($sp)
lw  $25, %call16(func1)($gp)
jr  $25
move   $5, $1

Whether the load of 16(sp) comes before the load of 20(sp) doesn't seem to matter to me. I'll modify the test to expose the bug better, as it should be using -relocation-model=pic.

[sdardis]

Thanks, committed as r282063.

lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   }
 
   SmallVector<SDValue, 8> Ops(1, Chain);
   SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
 
   getOpndList(Ops, RegsToPass, IsPICCall, GlobalOrExternal, InternalLinkage,
               IsCallReloc, CLI, Callee, Chain);
 
-  if (IsTailCall)
+  if (IsTailCall) {
+    MF.getFrameInfo().setHasTailCall();
     return DAG.getNode(MipsISD::TailCall, DL, MVT::Other, Ops);
+  }
 
   Chain = DAG.getNode(MipsISD::JmpLink, DL, NodeTys, Ops);
   SDValue InFlag = Chain.getValue(1);
 
   // Create the CALLSEQ_END node.
   Chain = DAG.getCALLSEQ_END(Chain, NextStackOffsetVal,
                              DAG.getIntPtrConstant(0, DL, true), InFlag, DL);
   InFlag = Chain.getValue(1);

test/CodeGen/Mips/tailcall/tail-call-arguments-clobber.ll

+; RUN: llc -march=mips -mcpu=mips32 -O3 < %s | FileCheck %s \
+; RUN:     -check-prefix=MIPS32
+; RUN: llc -march=mips64 -mcpu=mips64 -target-abi n64 < %s | FileCheck %s \
+; RUN:     -check-prefix=MIPS64
+; RUN: llc -march=mips64 -mcpu=mips64 -target-abi n32 < %s | FileCheck %s \
+; RUN:     -check-prefix=MIPS64
+
+
+; LLVM PR/30197
+; Test that the scheduler does not order loads and stores of arguments that
+; are passed on the stack such that the arguments of the caller are clobbered
+; too early.
+
+; O32 case: The last two arguments should appear at 16(sp), 20(sp). The order
+;           of the loads doesn't matter, but they have to become before the
+;           stores
+declare i32 @func2(i32, i32, i32, i32, i32, i32)
+
+define i32 @func1(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f){
+
+; MIPS32: lw ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS32: lw ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS32: sw ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS32: sw ${{[0-9]+}}, {{[0-9]+}}($sp)
+  %retval = tail call i32 @func1(i32 %a, i32 %f, i32 %c, i32 %d, i32 %e, i32 %b)
+
+  ret i32 %retval
+}
+
+; N64, N32 cases: N64 and N32 both pass 8 arguments in registers. The order
+;           of the loads doesn't matter, but they have to become before the
+;           stores
+
+declare i64 @func4(i64, i64, i64, i64, i64, i64, i64, i64, i64, i64)
+
+define i64 @func3(i64 %a, i64 %b, i64 %c, i64 %d,
+                  i64 %e, i64 %f, i64 %g, i64 %h,
+                  i64 %i, i64 %j){
+
+; MIPS64: ld ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS64: ld ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS64: sd ${{[0-9]+}}, {{[0-9]+}}($sp)
+; MIPS64: sd ${{[0-9]+}}, {{[0-9]+}}($sp)
+  %retval = tail call i64 @func4(i64 %a, i64 %j, i64 %c, i64 %d, i64 %e, i64 %f, i64 %g, i64 %h, i64 %i, i64 %b)
+
+  ret i64 %retval
+}
+
+