[mips] Reordering callseq* nodes to be linear

[abeserminji]

Summary

When calling a function and passing large argument by value, and when argument size is above certain threshold, memcpy is used to copy argument on stack instead of sequence of loads and stores. In that case callseq* nodes for memcpy are nested inside callseq* nodes for the called function. This patch corrects this behavior by moving callseq_start of the called function after arguments calculation to temporary registers, so that callseq* nodes in resulting DAG are linear.

[sdardis]

+llvm-commits

That list should have been added when the patch was uploaded.

Can you re-upload the patch with full context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Can you also add -verify-machineinstrs to test/CodeGen/Mips/largeimmprinting.ll ? This is the existing test case that breaks the machine verifier.

test/CodeGen/Mips/callseq_order.ll

18–52

Rather than matching SelectionDAG's output, can you instead match the end output of -debug-only=isel and ensure that the output is a sequence of ADJCALLSTACKDOWN, ADJCALLSTACKUP which are not nested and match the memcpy calls where they occur.

[abeserminji]

Comments resolved.

[abeserminji]

Forgot to upload patch with full context.

[sdardis]

test/CodeGen/Mips/callseq_order.ll

2–9

Add -verify-machineinstrs to these llc commands.

2–9

These can have -stop-before=expand-isel-pseudos to terminate llc early.

16

This and the one below can be removed.

17

The #0 and the #1s below can be removed.

[abeserminji]

Comments resolved.

[sdardis]

LGTM with nits addressed.

test/CodeGen/Mips/callseq_order.ll

2

You can drop the CPU specification portion of the llc invocations here, the defaults of 32r2 and 64r2 should sufficient to test the logic that has been changed.

test/CodeGen/Mips/llvm-ir/mul.ll

271

You don't need to bind the register number to a FileCheck variable in this case, as it's unused afterwards. Either match it with {{[0-9a-z]+}} or drop the register portion as we're interested in matching the:

lw $25, %call16(__multi3)

part of the instruction. This applies to the following test changes as well.

[abeserminji]

Comments resolved.

lib/Target/Mips/MipsISelLowering.h

                        MipsCCState &State) const;
 
     /// passByValArg - Pass a byval argument in registers or on stack.
-    void passByValArg(SDValue Chain, const SDLoc &DL,
+    SDValue passByValArg(SDValue Chain, const SDLoc &DL,
                          std::deque<std::pair<unsigned, SDValue>> &RegsToPass,
-                      SmallVectorImpl<SDValue> &MemOpChains, SDValue StackPtr,
-                      MachineFrameInfo &MFI, SelectionDAG &DAG, SDValue Arg,
-                      unsigned FirstReg, unsigned LastReg,
-                      const ISD::ArgFlagsTy &Flags, bool isLittle,
-                      const CCValAssign &VA) const;
+                         SmallVectorImpl<SDValue> &MemOpChains,
+                         SDValue StackPtr, MachineFrameInfo &MFI,
+                         SelectionDAG &DAG, SDValue Arg, unsigned FirstReg,
+                         unsigned LastReg, const ISD::ArgFlagsTy &Flags,
+                         bool isLittle, const CCValAssign &VA) const;
 
     /// writeVarArgRegs - Write variable function arguments passed in registers
     /// to the stack. Also create a stack frame object for the first variable

lib/Target/Mips/MipsISelLowering.cpp

   if (IsTailCall)
     ++NumTailCalls;
 
-  // Chain is the output chain of the last Load/Store or CopyToReg node.
-  // ByValChain is the output chain of the last Memcpy node created for copying
-  // byval arguments to the stack.
-  unsigned StackAlignment = TFL->getStackAlignment();
-  NextStackOffset = alignTo(NextStackOffset, StackAlignment);
-  SDValue NextStackOffsetVal = DAG.getIntPtrConstant(NextStackOffset, DL, true);
-
-  if (!IsTailCall)
-    Chain = DAG.getCALLSEQ_START(Chain, NextStackOffset, 0, DL);
-
   SDValue StackPtr =
       DAG.getCopyFromReg(Chain, DL, ABI.IsN64() ? Mips::SP_64 : Mips::SP,
                          getPointerTy(DAG.getDataLayout()));

       assert(ByValIdx < CCInfo.getInRegsParamsCount());
       assert(!IsTailCall &&
              "Do not tail-call optimize if there is a byval argument.");
-      passByValArg(Chain, DL, RegsToPass, MemOpChains, StackPtr, MFI, DAG, Arg,
-                   FirstByValReg, LastByValReg, Flags, Subtarget.isLittle(),
-                   VA);
+      Chain = passByValArg(Chain, DL, RegsToPass, MemOpChains, StackPtr, MFI,
+                           DAG, Arg, FirstByValReg, LastByValReg, Flags,
+                           Subtarget.isLittle(), VA);
       CCInfo.nextInRegsParam();
       continue;
     }

   EVT Ty = Callee.getValueType();
   bool GlobalOrExternal = false, IsCallReloc = false;
 
+  // Chain is the output chain of the last Load/Store or CopyToReg node.
+  // ByValChain is the output chain of the last Memcpy node created for copying
+  // byval arguments to the stack.
+  unsigned StackAlignment = TFL->getStackAlignment();
+  NextStackOffset = alignTo(NextStackOffset, StackAlignment);
+  SDValue NextStackOffsetVal = DAG.getIntPtrConstant(NextStackOffset, DL, true);
+
+  if (!IsTailCall)
+    Chain = DAG.getCALLSEQ_START(Chain, NextStackOffset, 0, DL);
+
   // The long-calls feature is ignored in case of PIC.
   // While we do not support -mshared / -mno-shared properly,
   // ignore long-calls in case of -mabicalls too.

 }
 
 // Copy byVal arg to registers and stack.
-void MipsTargetLowering::passByValArg(
+SDValue MipsTargetLowering::passByValArg(
     SDValue Chain, const SDLoc &DL,
     std::deque<std::pair<unsigned, SDValue>> &RegsToPass,
     SmallVectorImpl<SDValue> &MemOpChains, SDValue StackPtr,

 
     // Return if the struct has been fully copied.
     if (ByValSizeInBytes == OffsetInBytes)
-      return;
+      return Chain;
 
     // Copy the remainder of the byval argument with sub-word loads and shifts.
     if (LeftoverBytes) {

 
       unsigned ArgReg = ArgRegs[FirstReg + I];
       RegsToPass.push_back(std::make_pair(ArgReg, Val));
-      return;
+      return Chain;
     }
   }
 

                             DAG.getConstant(OffsetInBytes, DL, PtrTy));
   SDValue Dst = DAG.getNode(ISD::ADD, DL, PtrTy, StackPtr,
                             DAG.getIntPtrConstant(VA.getLocMemOffset(), DL));
-  Chain = DAG.getMemcpy(Chain, DL, Dst, Src,
-                        DAG.getConstant(MemCpySize, DL, PtrTy),
-                        Alignment, /*isVolatile=*/false, /*AlwaysInline=*/false,
-                        /*isTailCall=*/false,
-                        MachinePointerInfo(), MachinePointerInfo());
+  Chain = DAG.getMemcpy(
+      Chain, DL, Dst, Src, DAG.getConstant(MemCpySize, DL, PtrTy), Alignment,
+      /*isVolatile=*/false, /*AlwaysInline=*/false,
+      /*isTailCall=*/false, MachinePointerInfo(), MachinePointerInfo());
   MemOpChains.push_back(Chain);
+
+  return Chain;
 }
 
 void MipsTargetLowering::writeVarArgRegs(std::vector<SDValue> &OutChains,

test/CodeGen/Mips/callseq_order.ll

+; RUN: llc -mtriple=mipsel-linux-gnu -mcpu=mips32 -o /dev/null \
+; RUN:     --debug %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips64el-linux-gnu -mcpu=mips64 -o /dev/null \
+; RUN:     --debug %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips-linux-gnu -mcpu=mips32 -o /dev/null \
+; RUN:     --debug %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips64-linux-gnu -mcpu=mips64 -o /dev/null \
+; RUN:     --debug %s 2>&1 | FileCheck %s
+
+
+%struct.Str1 = type { [64 x i32] }
+
+@s1 = common global %struct.Str1 zeroinitializer, align 4
+
+; Function Attrs: noinline nounwind optnone
+define void @foo1() #0 {
+entry:
+  call void @bar1(%struct.Str1* byval align 4 @s1)
+  ret void
+  ; CHECK-LABEL: Optimized legalized selection DAG: BB#0 'foo1:entry'
+  ; CHECK: t[[T0:[0-9]+]]: ch,glue = callseq_start
+  ; CHECK: t[[T1:[0-9]+]]: ch,glue = MipsISD::JmpLink t[[T0]]
+  ; CHECK: t[[T2:[0-9]+]]: ch,glue = callseq_end t[[T1]]
+  ; CHECK: t[[T3:[0-9]+]]: i{{(32|64)}},ch,glue = CopyFromReg t[[T2]]
+  ; CHECK: t[[T4:[0-9]+]]: ch = TokenFactor {{(t[0-9]+:1, )+}}t[[T3]]:1
+  ; CHECK: t[[T5:[0-9]+]]: ch,glue = callseq_start t[[T4]]
+  ; CHECK: t[[T6:[0-9]+]]: ch,glue = MipsISD::JmpLink t[[T5]]
+  ; CHECK: callseq_end t[[T6]]
+  ; CHECK-LABEL: Instruction selection begins: BB#0 'entry'
+}
+
+declare void @bar1(%struct.Str1* byval align 4) #1
+
+; Function Attrs: noinline nounwind optnone
+define void @foo2() #0 {
+entry:
+  call void @bar2(%struct.Str1* byval align 4 @s1, %struct.Str1* byval align 4 @s1)
+  ret void
+  ; CHECK-LABEL: Optimized legalized selection DAG: BB#0 'foo2:entry'
+  ; CHECK: t[[T0:[0-9]+]]: ch,glue = callseq_start
+  ; CHECK: t[[T1:[0-9]+]]: ch,glue = MipsISD::JmpLink t[[T0]]
+  ; CHECK: t[[T2:[0-9]+]]: ch,glue = callseq_end t[[T1]]
+  ; CHECK: t[[T3:[0-9]+]]: i{{(32|64)}},ch,glue = CopyFromReg t[[T2]]
+  ; CHECK: t[[T4:[0-9]+]]: ch,glue = callseq_start t[[T3]]
+  ; CHECK: t[[T5:[0-9]+]]: ch,glue = MipsISD::JmpLink t[[T4]]
+  ; CHECK: t[[T6:[0-9]+]]: ch,glue = callseq_end t[[T5]]
+  ; CHECK: t[[T7:[0-9]+]]: i{{(32|64)}},ch,glue = CopyFromReg t[[T6]]
+  ; CHECK: t[[T8:[0-9]+]]: ch = TokenFactor {{(t[0-9]+:1, )+}}t[[T3]]:1, t[[T7]]:1
+  ; CHECK: t[[T9:[0-9]+]]: ch,glue = callseq_start t[[T8]]
+  ; CHECK: t[[T10:[0-9]+]]: ch,glue = MipsISD::JmpLink t[[T9]]
+  ; CHECK: callseq_end t[[T10]]
+  ; CHECK-LABEL: Instruction selection begins: BB#0 'entry'
+}
+
+declare void @bar2(%struct.Str1* byval align 4, %struct.Str1* byval align 4) #1
+

test/CodeGen/Mips/llvm-ir/mul.ll

   ; MM64R6:         daddu   $2, $[[T1]], $[[T0]]
   ; MM64R6-DAG:     dmul    $3, $5, $7
 
-  ; MM32:           lw      $25, %call16(__multi3)($16)
+  ; MM32:           lw      $25, %call16(__multi3)($[[T0:[0-9]+]])
 
   %r = mul i128 %a, %b
   ret i128 %r

test/CodeGen/Mips/llvm-ir/sdiv.ll

   ; GP64-NOT-R6:  ld      $25, %call16(__divti3)($gp)
   ; 64R6:         ld      $25, %call16(__divti3)($gp)
 
-  ; MM32:         lw      $25, %call16(__divti3)($16)
+  ; MM32:         lw      $25, %call16(__divti3)($[[T0:[0-9]+]])
 
   ; MM64:         ld      $25, %call16(__divti3)($2)
 

test/CodeGen/Mips/llvm-ir/srem.ll

   ; GP64-NOT-R6:  ld      $25, %call16(__modti3)($gp)
   ; 64R6:         ld      $25, %call16(__modti3)($gp)
 
-  ; MM32:         lw      $25, %call16(__modti3)($16)
+  ; MM32:         lw      $25, %call16(__modti3)($[[T0:[0-9]+]])
 
   ; MM64:         ld      $25, %call16(__modti3)($2)
 

test/CodeGen/Mips/llvm-ir/udiv.ll

   ; GP64-NOT-R6:  ld      $25, %call16(__udivti3)($gp)
   ; 64-R6:        ld      $25, %call16(__udivti3)($gp)
 
-  ; MM32:         lw      $25, %call16(__udivti3)($16)
+  ; MM32:         lw      $25, %call16(__udivti3)($[[T0:[0-9]+]])
 
   ; MM64:         ld      $25, %call16(__udivti3)($2)
 

test/CodeGen/Mips/llvm-ir/urem.ll

   ; GP64-NOT-R6:  ld      $25, %call16(__umodti3)($gp)
   ; 64R6:         ld      $25, %call16(__umodti3)($gp)
 
-  ; MM32:         lw      $25, %call16(__umodti3)($16)
+  ; MM32:         lw      $25, %call16(__umodti3)($[[T0:[0-9]+]])
 
   ; MM64:         ld      $25, %call16(__umodti3)($2)