[mips] Prevent shrink-wrap for BuildPairF64, ExtractElementF64 when they use $sp

[vstefanovic]

Summary

For a certain combination of options, BuildPairF64_{64}, ExtractElementF64{_64}
may be expanded into instructions using stack.
Add implicit operand $sp for such cases so that ShrinkWrapping doesn't move
prologue setup below them.

Fixes MultiSource/Benchmarks/MallocBench/cfrac for
'--target=mips-img-linux-gnu -mcpu=mips32r6 -mfpxx -mnan=2008'
and
'--target=mips-img-linux-gnu -mcpu=mips32r6 -mfp64 -mnan=2008 -mno-odd-spreg'.

[thegameg]

This looks ok, but I am not sure the FrameSetup flag was made to be used in this way.

Would it make more sense to add SP as an implicit use to the instruction instead? I believe this is how things are usually modelled now, but I might be wrong.

test/CodeGen/Mips/shrink-wrap-buildpairf64-extractelementf64.ll

1 ↗

(On Diff #161524)

Can you please add a MIR test with -start-before shrink-wrap -stop-after prologepilog instead of an IR test?

[MatzeB]

This looks wrong to me. And it would be good if you could explain with more detail or an example. So far I see: An instruction uses a register but we didn't add a register operand for that. Why would we fix this by adding an exception into shrink wrapping instead of making sure there is an actual operand?

[vstefanovic]

Adding implicit operand $sp to the instructions instead of FrameSetup flag.

[vstefanovic]

Thanks for the comments, adding $sp as implicit operand is a lot better, no need to modify ShrinkWrapping then.

An example - BuildPair64 loads 64 bits into a FPR from two GPRs.

$d0 = BuildPairF64 $zero, $zero

This is later usually expanded to mtc1 + mthc1:

$f0 = MTC1 $zero
$d0 = MTHC1_D32 $d0, $zero

but when mthc1 isn't available (e.g. for '-mfpxx -mcpu=mips32'), $d0 is loaded via stack:

SW $zero, $sp, 16
SW $zero, $sp, 20
$d0 = LDC1 $sp, 16

(I'll update commit message if this is ok.)

test/CodeGen/Mips/shrink-wrap-buildpairf64-extractelementf64.ll

1 ↗

(On Diff #161524)

I also need to check whether $sp is added to implicit operands in the isel pass.
There could be a separate test for that, but I found it more convenient to cover everything with one input.

[thegameg]

test/CodeGen/Mips/shrink-wrap-buildpairf64-extractelementf64.ll

1 ↗

(On Diff #161524)

Yes, I think just -stop-after=isel should do it.

[vstefanovic]

Split the test.

[thegameg]

Looks better now. Thanks for adding the MIR test.

LGTM although I have no knowledge about the MIPS backend, so you might want to wait for someone else.

[atanasyan]

LGTM

llvm/trunk/lib/Target/Mips/MipsSEFrameLowering.cpp

bool ExpandPseudo::expandBuildPairF64(MachineBasicBlock &MBB,
   // because it never creates a BuildPairF64 node.
   //
   // The FP64A ABI (fp64 with nooddspreg) must also use a spill/reload sequence
   // for odd-numbered double precision values (because the lower 32-bits is
   // transferred with mtc1 which is redirected to the upper half of the even
   // register). Unfortunately, we have to make this decision before register
   // allocation so for now we use a spill/reload sequence for all
   // double-precision values in regardless of being an odd/even register.
-  if ((Subtarget.isABI_FPXX() && !Subtarget.hasMTHC1()) ||
-      (FP64 && !Subtarget.useOddSPReg())) {
+  //
+  // For the cases that should be covered here MipsSEISelDAGToDAG adds $sp as
+  // implicit operand, so other passes (like ShrinkWrapping) are aware that
+  // stack is used.
+  if (I->getNumOperands() == 4 && I->getOperand(3).isReg()
+      && I->getOperand(3).getReg() == Mips::SP) {
     unsigned DstReg = I->getOperand(0).getReg();
     unsigned LoReg = I->getOperand(1).getReg();
     unsigned HiReg = I->getOperand(2).getReg();
 
     // It should be impossible to have FGR64 on MIPS-II or MIPS32r1 (which are
     // the cases where mthc1 is not available). 64-bit architectures and
     // MIPS32r2 or later can use FGR64 though.
     assert(Subtarget.isGP64bit() || Subtarget.hasMTHC1() ||
bool ExpandPseudo::expandExtractElementF64(MachineBasicBlock &MBB,
   // because it never creates a ExtractElementF64 node.
   //
   // The FP64A ABI (fp64 with nooddspreg) must also use a spill/reload sequence
   // for odd-numbered double precision values (because the lower 32-bits is
   // transferred with mfc1 which is redirected to the upper half of the even
   // register). Unfortunately, we have to make this decision before register
   // allocation so for now we use a spill/reload sequence for all
   // double-precision values in regardless of being an odd/even register.
-
-  if ((Subtarget.isABI_FPXX() && !Subtarget.hasMTHC1()) ||
-      (FP64 && !Subtarget.useOddSPReg())) {
+  //
+  // For the cases that should be covered here MipsSEISelDAGToDAG adds $sp as
+  // implicit operand, so other passes (like ShrinkWrapping) are aware that
+  // stack is used.
+  if (I->getNumOperands() == 4 && I->getOperand(3).isReg()
+      && I->getOperand(3).getReg() == Mips::SP) {
     unsigned DstReg = I->getOperand(0).getReg();
     unsigned SrcReg = Op1.getReg();
     unsigned N = Op2.getImm();
     int64_t Offset = 4 * (Subtarget.isLittle() ? N : (1 - N));
 
     // It should be impossible to have FGR64 on MIPS-II or MIPS32r1 (which are
     // the cases where mfhc1 is not available). 64-bit architectures and
     // MIPS32r2 or later can use FGR64 though.

llvm/trunk/lib/Target/Mips/MipsSEISelDAGToDAG.cpp

for (auto &MBB: MF) {
     for (auto &MI: MBB) {
       switch (MI.getOpcode()) {
       case Mips::RDDSP:
         addDSPCtrlRegOperands(false, MI, MF);
         break;
       case Mips::WRDSP:
         addDSPCtrlRegOperands(true, MI, MF);
         break;
+      case Mips::BuildPairF64_64:
+      case Mips::ExtractElementF64_64:
+        if (!Subtarget->useOddSPReg()) {
+          MI.addOperand(MachineOperand::CreateReg(Mips::SP, false, true));
+          break;
+        }
+      // fallthrough
+      case Mips::BuildPairF64:
+      case Mips::ExtractElementF64:
+        if (Subtarget->isABI_FPXX() && !Subtarget->hasMTHC1())
+          MI.addOperand(MachineOperand::CreateReg(Mips::SP, false, true));
+        break;
       default:
         replaceUsesWithZeroReg(MRI, MI);
       }
     }
   }
 }
 
 void MipsSEDAGToDAGISel::selectAddE(SDNode *Node, const SDLoc &DL) const {

llvm/trunk/test/CodeGen/Mips/buildpairf64-extractelementf64-implicit-sp.ll

+; RUN: llc -o - %s -mtriple=mips-unknown-linux-gnu \
+; RUN:     -mcpu=mips32 -mattr=+fpxx \
+; RUN:     -stop-after=expand-isel-pseudos | \
+; RUN:     FileCheck %s -check-prefix=FPXX-IMPLICIT-SP
+
+; RUN: llc -o - %s -mtriple=mips-unknown-linux-gnu \
+; RUN:     -mcpu=mips32r6 -mattr=+fp64,+nooddspreg \
+; RUN:     -stop-after=expand-isel-pseudos | \
+; RUN:     FileCheck %s -check-prefix=FP64-IMPLICIT-SP
+
+; RUN: llc -o - %s -mtriple=mips-unknown-linux-gnu \
+; RUN:     -mcpu=mips32r2 -mattr=+fpxx \
+; RUN:     -stop-after=expand-isel-pseudos | \
+; RUN:     FileCheck %s -check-prefix=NO-IMPLICIT-SP
+
+define double @foo2(i32 signext %v1, double %d1) {
+entry:
+; FPXX-IMPLICIT-SP: BuildPairF64 %{{[0-9]+}}, %{{[0-9]+}}, implicit $sp
+; FPXX-IMPLICIT-SP: ExtractElementF64 killed %{{[0-9]+}}, 1, implicit $sp
+; FP64-IMPLICIT-SP: BuildPairF64_64 %{{[0-9]+}}, %{{[0-9]+}}, implicit $sp
+; FP64-IMPLICIT-SP: ExtractElementF64_64 killed %{{[0-9]+}}, 1, implicit $sp
+; NO-IMPLICIT-SP: BuildPairF64 %{{[0-9]+}}, %{{[0-9]+}}
+; NO-IMPLICIT-SP-NOT: BuildPairF64 %{{[0-9]+}}, %{{[0-9]+}}, implicit $sp
+; NO-IMPLICIT-SP: ExtractElementF64 killed %{{[0-9]+}}, 1
+; NO-IMPLICIT-SP-NOT: ExtractElementF64 killed %{{[0-9]+}}, 1, implicit $sp
+  %conv = fptrunc double %d1 to float
+  %0 = tail call float @llvm.copysign.f32(float 1.000000e+00, float %conv)
+  %conv1 = fpext float %0 to double
+  ret double %conv1
+}
+
+declare float @llvm.copysign.f32(float, float)

llvm/trunk/test/CodeGen/Mips/shrink-wrap-buildpairf64-extractelementf64.mir

+# RUN: llc -o - %s -mtriple=mips-unknown-linux-gnu -enable-shrink-wrap=true \
+# RUN:     -start-before=shrink-wrap -stop-after=prologepilog | FileCheck %s
+
+--- |
+  declare void @foo()
+  define void @testBuildPairF64() {
+    ret void
+  }
+  define void @testBuildPairF64_64() {
+    ret void
+  }
+  define void @testBuildPairF64implicitSp() {
+    ret void
+  }
+  define void @testBuildPairF64_64implicitSp() {
+    ret void
+  }
+  define void @testExtractElementF64() {
+    ret void
+  }
+  define void @testExtractElementF64_64() {
+    ret void
+  }
+  define void @testExtractElementF64implicitSp() {
+    ret void
+  }
+  define void @testExtractElementF64_64implicitSp() {
+    ret void
+  }
+...
+---
+name: testBuildPairF64
+# CHECK-LABEL: name: testBuildPairF64
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: BuildPairF64
+body: |
+  bb.0:
+    $d0 = BuildPairF64 $zero, $zero
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testBuildPairF64_64
+# CHECK-LABEL: name: testBuildPairF64_64
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: BuildPairF64_64
+body: |
+  bb.0:
+    $d0_64 = BuildPairF64_64 $zero, $zero
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testBuildPairF64implicitSp
+# CHECK-LABEL: name: testBuildPairF64implicitSp
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: $sp = ADDiu $sp, -{{[0-9]+}}
+body: |
+  bb.0:
+    $d0 = BuildPairF64 $zero, $zero, implicit $sp
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testBuildPairF64_64implicitSp
+# CHECK-LABEL: name: testBuildPairF64_64implicitSp
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: $sp = ADDiu $sp, -{{[0-9]+}}
+body: |
+  bb.0:
+    $d0_64 = BuildPairF64_64 $zero, $zero, implicit $sp
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testExtractElementF64
+# CHECK-LABEL: name: testExtractElementF64
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: ExtractElementF64
+body: |
+  bb.0:
+    $at = ExtractElementF64 $d6, 1
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testExtractElementF64_64
+# CHECK-LABEL: name: testExtractElementF64_64
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: ExtractElementF64_64
+body: |
+  bb.0:
+    $at = ExtractElementF64_64 $d12_64, 1
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testExtractElementF64implicitSp
+# CHECK-LABEL: name: testExtractElementF64implicitSp
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: $sp = ADDiu $sp, -{{[0-9]+}}
+body: |
+  bb.0:
+    $at = ExtractElementF64 $d6, 1, implicit $sp
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...
+---
+name: testExtractElementF64_64implicitSp
+# CHECK-LABEL: name: testExtractElementF64_64implicitSp
+# CHECK: bb.0
+# CHECK-NEXT: successors
+# CHECK-NEXT: {{[[:space:]]$}}
+# CHECK-NEXT: $sp = ADDiu $sp, -{{[0-9]+}}
+body: |
+  bb.0:
+    $at = ExtractElementF64_64 $d12_64, 1, implicit $sp
+  bb.1:
+    JAL @foo, implicit-def $ra
+  bb.2:
+    RetRA
+...