[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

19–53

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

void copyByValRegs(SDValue Chain, const SDLoc &DL,
                        std::vector<SDValue> &OutChains, SelectionDAG &DAG,
                        const ISD::ArgFlagsTy &Flags,
                        SmallVectorImpl<SDValue> &InVals,
                        const Argument *FuncArg, unsigned FirstReg,
                        unsigned LastReg, const CCValAssign &VA,
                        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
     /// argument.
     void writeVarArgRegs(std::vector<SDValue> &OutChains, SDValue Chain,
                          const SDLoc &DL, SelectionDAG &DAG,
                          CCState &State) const;
 

lib/Target/Mips/MipsISelLowering.cpp

MipsTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   }
   if (!IsTailCall && CLI.CS && CLI.CS.isMustTailCall())
     report_fatal_error("failed to perform tail call elimination on a call "
                        "site marked musttail");
 
   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()));
 
   std::deque<std::pair<unsigned, SDValue>> RegsToPass;
   SmallVector<SDValue, 8> MemOpChains;
 
   CCInfo.rewindByValRegsInfo();
if (Flags.isByVal()) {
       unsigned ByValIdx = CCInfo.getInRegsParamsProcessed();
       CCInfo.getInRegsParamInfo(ByValIdx, FirstByValReg, LastByValReg);
 
       assert(Flags.getByValSize() &&
              "ByVal args of size 0 should have been ignored by front-end.");
       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,
+      Chain = passByValArg(Chain, DL, RegsToPass, MemOpChains, StackPtr, MFI, DAG, Arg,
                    FirstByValReg, LastByValReg, Flags, Subtarget.isLittle(),
                    VA);
       CCInfo.nextInRegsParam();
       continue;
     }
 
     // Promote the value if needed.
     switch (VA.getLocInfo()) {
MipsTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   // If the callee is a GlobalAddress/ExternalSymbol node (quite common, every
   // direct call is) turn it into a TargetGlobalAddress/TargetExternalSymbol
   // node so that legalize doesn't hack it.
 
   SDValue CalleeLo;
   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.
   if (!Subtarget.isABICalls() && !IsPIC) {
     // If the function should be called using "long call",
     // get its address into a register to prevent using
     // of the `jal` instruction for the direct call.
     if (auto *N = dyn_cast<ExternalSymbolSDNode>(Callee)) {
SDValue StorePtr = DAG.getNode(ISD::ADD, DL, PtrTy, FIN,
                                    DAG.getConstant(Offset, DL, PtrTy));
     SDValue Store = DAG.getStore(Chain, DL, DAG.getRegister(VReg, RegTy),
                                  StorePtr, MachinePointerInfo(FuncArg, Offset));
     OutChains.push_back(Store);
   }
 }
 
 // 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,
     MachineFrameInfo &MFI, SelectionDAG &DAG, SDValue Arg, unsigned FirstReg,
     unsigned LastReg, const ISD::ArgFlagsTy &Flags, bool isLittle,
     const CCValAssign &VA) const {
   unsigned ByValSizeInBytes = Flags.getByValSize();
   unsigned OffsetInBytes = 0; // From beginning of struct
for (; I < NumRegs - LeftoverBytes; ++I, OffsetInBytes += RegSizeInBytes) {
                                     MachinePointerInfo(), Alignment);
       MemOpChains.push_back(LoadVal.getValue(1));
       unsigned ArgReg = ArgRegs[FirstReg + I];
       RegsToPass.push_back(std::make_pair(ArgReg, LoadVal));
     }
 
     // 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) {
       SDValue Val;
 
       for (unsigned LoadSizeInBytes = RegSizeInBytes / 2, TotalBytesLoaded = 0;
            OffsetInBytes < ByValSizeInBytes; LoadSizeInBytes /= 2) {
         unsigned RemainingSizeInBytes = ByValSizeInBytes - OffsetInBytes;
if (LeftoverBytes) {
 
         OffsetInBytes += LoadSizeInBytes;
         TotalBytesLoaded += LoadSizeInBytes;
         Alignment = std::min(Alignment, LoadSizeInBytes);
       }
 
       unsigned ArgReg = ArgRegs[FirstReg + I];
       RegsToPass.push_back(std::make_pair(ArgReg, Val));
-      return;
+      return Chain;
     }
   }
 
   // Copy remainder of byval arg to it with memcpy.
   unsigned MemCpySize = ByValSizeInBytes - OffsetInBytes;
   SDValue Src = DAG.getNode(ISD::ADD, DL, PtrTy, Arg,
                             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,
                                          SDValue Chain, const SDLoc &DL,
                                          SelectionDAG &DAG,
                                          CCState &State) const {
   ArrayRef<MCPhysReg> ArgRegs = ABI.GetVarArgRegs();
   unsigned Idx = State.getFirstUnallocated(ArgRegs);

test/CodeGen/Mips/callseq_order.ll

+; RUN: llc -mtriple=mipsel-linux-gnu -o /dev/null                 \
+; RUN:     -verify-machineinstrs -stop-before=expand-isel-pseudos \
+; RUN:     -debug-only=isel %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips64el-linux-gnu -o /dev/null               \
+; RUN:     -verify-machineinstrs -stop-before=expand-isel-pseudos \
+; RUN:     -debug-only=isel %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips-linux-gnu -o /dev/null                   \
+; RUN:     -verify-machineinstrs -stop-before=expand-isel-pseudos \
+; RUN:     -debug-only=isel %s 2>&1 | FileCheck %s
+; RUN: llc -mtriple=mips64-linux-gnu -o /dev/null                 \
+; RUN:     -verify-machineinstrs -stop-before=expand-isel-pseudos \
+; RUN:     -debug-only=isel %s 2>&1 | FileCheck %s
+
+
+%struct.Str1 = type { [64 x i32] }
+
+@s1 = common global %struct.Str1 zeroinitializer, align 4
+
+define void @foo1() {
+entry:
+  call void @bar1(%struct.Str1* byval align 4 @s1)
+  ret void
+  ; CHECK-LABEL: *** MachineFunction at end of ISel ***
+  ; CHECK-LABEL: # Machine code for function foo1: IsSSA, TracksLiveness
+  ; CHECK: ADJCALLSTACKDOWN
+  ; CHECK: JAL <es:memcpy>
+  ; CHECK: ADJCALLSTACKUP
+  ; CHECK: ADJCALLSTACKDOWN
+  ; CHECK: JAL <ga:@bar1>
+  ; CHECK: ADJCALLSTACKUP
+  ; CHECK-LABEL: # End machine code for function foo1.
+}
+
+declare void @bar1(%struct.Str1* byval align 4)
+
+define void @foo2() {
+entry:
+  call void @bar2(%struct.Str1* byval align 4 @s1, %struct.Str1* byval align 4 @s1)
+  ret void
+  ; CHECK-LABEL: *** MachineFunction at end of ISel ***
+  ; CHECK-LABEL: # Machine code for function foo2: IsSSA, TracksLiveness
+  ; CHECK: ADJCALLSTACKDOWN
+  ; CHECK: JAL <es:memcpy>
+  ; CHECK: ADJCALLSTACKUP
+  ; CHECK: ADJCALLSTACKDOWN
+  ; CHECK: JAL <es:memcpy>
+  ; CHECK: ADJCALLSTACKUP
+  ; CHECK: ADJCALLSTACKDOWN
+  ; CHECK: JAL <ga:@bar2>
+  ; CHECK: ADJCALLSTACKUP
+  ; CHECK-LABEL: # End machine code for function foo2.
+}
+
+declare void @bar2(%struct.Str1* byval align 4, %struct.Str1* byval align 4)
+

test/CodeGen/Mips/largeimmprinting.ll

-; RUN: llc -march=mipsel -relocation-model=pic < %s | FileCheck %s -check-prefix=32
-; RUN: llc -march=mips64el -mcpu=mips4 -target-abi=n64 -relocation-model=pic < %s | \
-; RUN:     FileCheck %s -check-prefix=64
-; RUN: llc -march=mips64el -mcpu=mips64 -target-abi=n64 -relocation-model=pic < %s | \
-; RUN:     FileCheck %s -check-prefix=64
+; RUN: llc -march=mipsel -relocation-model=pic  -verify-machineinstrs < %s |  \
+; RUN:     FileCheck %s -check-prefix=32
+; RUN: llc -march=mips64el -mcpu=mips4 -target-abi=n64 -relocation-model=pic  \
+; RUN:     -verify-machineinstrs < %s | FileCheck %s -check-prefix=64
+; RUN: llc -march=mips64el -mcpu=mips64 -target-abi=n64 -relocation-model=pic \
+; RUN:     -verify-machineinstrs < %s | FileCheck %s -check-prefix=64
 
 %struct.S1 = type { [65536 x i8] }
 
 @s1 = external global %struct.S1
 
 define void @f() nounwind {
 entry:
 ; 32:  lui     $[[R0:[0-9]+]], 1

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

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

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

 entry:
   ; ALL-LABEL: sdiv_i128:
 
   ; GP32:         lw      $25, %call16(__divti3)($gp)
 
   ; 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)
 
   ; MM64:         ld      $25, %call16(__divti3)($2)
 
   %r = sdiv i128 %a, %b
   ret i128 %r
 }

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

 entry:
 ; ALL-LABEL: srem_i128:
 
   ; GP32:         lw      $25, %call16(__modti3)($gp)
 
   ; 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)
 
   ; MM64:         ld      $25, %call16(__modti3)($2)
 
   %r = srem i128 %a, %b
   ret i128 %r
 }

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

 entry:
 ; ALL-LABEL: udiv_i128:
 
   ; GP32:         lw      $25, %call16(__udivti3)($gp)
 
   ; 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)
 
   ; MM64:         ld      $25, %call16(__udivti3)($2)
 
   %r = udiv i128 %a, %b
   ret i128 %r
 }

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

 entry:
   ; ALL-LABEL: urem_i128:
 
   ; GP32:         lw      $25, %call16(__umodti3)($gp)
 
   ; 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)
 
   ; MM64:         ld      $25, %call16(__umodti3)($2)
 
     %r = urem i128 %a, %b
     ret i128 %r
 }