Emit two CFI offset directives per double precision SDC1/LDC1 instead of just one for FR=1 registers

[jkolek]

Summary

This patch implements emission of two CFI offset directives (.cfi_offset) per instruction in FR=1 mode (in FR=0 mode emission of two directives per SDC1/LDC1 is already supported). Also, this patch modifies CSR_O32_FP64 to use only even FPU registers as callee-saved. After I repair the patch for FPXX (D4293) I will add test cases for FPXX .cfi_offset as well.

[vmedic]

lib/Target/Mips/MipsSEFrameLowering.cpp

342 ↗

(On Diff #10888)

This seems like duplicated check, if the register is part of FGR64 class isn't then FP unit 64 bit?

345 ↗

(On Diff #10888)

Not sure you can simply add 1 to dwarf register number, but I may be wrong.

351 ↗

(On Diff #10888)

This seems to use same value for both true/false case.

[dsanders]

lib/Target/Mips/MipsSEFrameLowering.cpp

342 ↗

(On Diff #10888)

Agreed. We only need to test for membership of FGR64RegClass. When !isFP64bit(), the register will never be in this class (it will be in AFGR64RegClass instead).

345 ↗

(On Diff #10888)

I was wondering the same thing. DWARF register numbers are arbitrary but I would be surprised if we hadn't allocated them in contiguous blocks.

It looks like it's ok though. According to MipsRegisterInfo.td we allocated a contiguous block starting at 32.

[jkolek]

In this patch I have removed STI.isFP64bit(), and I have added FPXX run lines in the test.

[jkolek]

Returning _Complex float or _Complex double must reside in two even numbered registers and not even+odd registers.

[dsanders]

lib/Target/Mips/MipsCallingConv.td

31 ↗

(On Diff #11051)

I believe this is correct. I can only find this in the N32 documentation but O32, N32, and N64 all do the same thing in gcc.

You need to update the comment above. It still says 'D1_64'.

Also, please add a complex float and complex double test to test/CodeGen/Mips/cconv/return-hard-float.ll.

32 ↗

(On Diff #11051)

I believe it to be a bug that we use D0 and D1 in this case. Testing with gcc showed it returning complex types in D0 and D2 for the O32 -mfp32 case too. It would be worth trying to link to a conj()/conjf() in a gcc-compiled glibc since I suspect that doesn't work at the moment.

test/CodeGen/Mips/cfi_offset.ll

16–19 ↗

(On Diff #11051)

No FileCheck commands use the 'CHECK' prefix because specifying any prefixes removes the default. I think you want multiple prefixes on the FileCheck commands '--check-prefix=CHECK --check-prefix=CHECK-EB'.

[vmedic]

lib/Target/Mips/MipsCallingConv.td

32 ↗

(On Diff #11051)

If this is an existing bug it may be worth leaving this part for the bug fixing phase, we are on a extremely tight schedule till tomorrow.

[dsanders]

lib/Target/Mips/MipsCallingConv.td

32 ↗

(On Diff #11051)

Yes it's an existing bug. I mentioned it because the line above fixes the same bug for the -mfp64 case. Deferring it to another patch seems reasonable to me.

[jkolek]

Fixed error in comment, test for complex values added.

[dsanders]

Thanks.

You still need to fix the unused 'CHECK' directives in test/CodeGen/Mips/cfi_offset.ll but once you've fixed that it will LGTM

test/CodeGen/Mips/cconv/return-hard-float.ll

51 ↗

(On Diff #11278)

Typo in 'Complex'

57 ↗

(On Diff #11278)

Here too

[jkolek]

Fixed CHECK test/CodeGen/Mips/cfi_offset.ll.

[dsanders]

LGTM

[dsanders]

Please fix the retComlpexDouble typo though.

[jkolek]

Typo fixed.

[dsanders]

Thanks. LGTM

[jkolek]

Closed by commit rL212769 (authored by zjovanovic).

llvm/trunk/lib/Target/Mips/MipsCallingConv.td

 // passing are defined in MipsISelLowering.cpp.
 def RetCC_MipsO32 : CallingConv<[
   // i32 are returned in registers V0, V1, A0, A1
   CCIfType<[i32], CCAssignToReg<[V0, V1, A0, A1]>>,
 
   // f32 are returned in registers F0, F2
   CCIfType<[f32], CCAssignToReg<[F0, F2]>>,
 
-  // f64 arguments are returned in D0_64 and D1_64 in FP64bit mode or
+  // f64 arguments are returned in D0_64 and D2_64 in FP64bit mode or
   // in D0 and D1 in FP32bit mode.
-  CCIfType<[f64], CCIfSubtarget<"isFP64bit()", CCAssignToReg<[D0_64, D1_64]>>>,
+  CCIfType<[f64], CCIfSubtarget<"isFP64bit()", CCAssignToReg<[D0_64, D2_64]>>>,
   CCIfType<[f64], CCIfSubtarget<"isNotFP64bit()", CCAssignToReg<[D0, D1]>>>
 ]>;
 
 //===----------------------------------------------------------------------===//
 // Mips N32/64 Calling Convention
 //===----------------------------------------------------------------------===//
 
 def CC_MipsN : CallingConv<[

 def CSR_O32_FPXX : CalleeSavedRegs<(add (sequence "D%u", 15, 10), RA, FP,
                                         (sequence "S%u", 7, 0))> {
   let OtherPreserved = (add (decimate (sequence "F%u", 30, 20), 2));
 }
 
 def CSR_O32 : CalleeSavedRegs<(add (sequence "D%u", 15, 10), RA, FP,
                                    (sequence "S%u", 7, 0))>;
 
-def CSR_O32_FP64 : CalleeSavedRegs<(add (sequence "D%u_64", 31, 20), RA, FP,
-                                        (sequence "S%u", 7, 0))>;
+def CSR_O32_FP64 :
+  CalleeSavedRegs<(add (decimate (sequence "D%u_64", 30, 20), 2), RA, FP,
 
 def CSR_N32 : CalleeSavedRegs<(add D20_64, D22_64, D24_64, D26_64, D28_64,
                                    D30_64, RA_64, FP_64, GP_64,
                                    (sequence "S%u_64", 7, 0))>;
 
 def CSR_N64 : CalleeSavedRegs<(add (sequence "D%u_64", 31, 24), RA_64, FP_64,
                                    GP_64, (sequence "S%u_64", 7, 0))>;
 
 def CSR_Mips16RetHelper :
   CalleeSavedRegs<(add V0, V1, FP,
                    (sequence "A%u", 3, 0), (sequence "S%u", 7, 0),
                    (sequence "D%u", 15, 10))>;

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

for (std::vector<CalleeSavedInfo>::const_iterator I = CSI.begin(),
             MCCFIInstruction::createOffset(nullptr, Reg0, Offset));
         BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
             .addCFIIndex(CFIIndex);
 
         CFIIndex = MMI.addFrameInst(
             MCCFIInstruction::createOffset(nullptr, Reg1, Offset + 4));
         BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
             .addCFIIndex(CFIIndex);
+      } else if (Mips::FGR64RegClass.contains(Reg)) {
+        unsigned Reg0 = MRI->getDwarfRegNum(Reg, true);
+        unsigned Reg1 = MRI->getDwarfRegNum(Reg, true) + 1;
+
+        if (!STI.isLittle())
+          std::swap(Reg0, Reg1);
+
+        unsigned CFIIndex = MMI.addFrameInst(
+          MCCFIInstruction::createOffset(nullptr, Reg0, Offset));
+        BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
+            .addCFIIndex(CFIIndex);
+
+        CFIIndex = MMI.addFrameInst(
+          MCCFIInstruction::createOffset(nullptr, Reg1, Offset + 4));
+        BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
+            .addCFIIndex(CFIIndex);
       } else {
         // Reg is either in GPR32 or FGR32.
         unsigned CFIIndex = MMI.addFrameInst(MCCFIInstruction::createOffset(
             nullptr, MRI->getDwarfRegNum(Reg, 1), Offset));
         BuildMI(MBB, MBBI, dl, TII.get(TargetOpcode::CFI_INSTRUCTION))
             .addCFIIndex(CFIIndex);
       }
     }

llvm/trunk/test/CodeGen/Mips/cconv/return-hard-float.ll

 ; RUN: llc -mtriple=mips-linux-gnu -relocation-model=static < %s | FileCheck --check-prefix=ALL --check-prefix=O32 %s
 ; RUN: llc -mtriple=mipsel-linux-gnu -relocation-model=static < %s | FileCheck --check-prefix=ALL --check-prefix=O32 %s
 
 ; RUN-TODO: llc -mtriple=mips64-linux-gnu -relocation-model=static -mattr=-n64,+o32 < %s | FileCheck --check-prefix=ALL --check-prefix=O32 %s
 ; RUN-TODO: llc -mtriple=mips64el-linux-gnu -relocation-model=static -mattr=-n64,+o32 < %s | FileCheck --check-prefix=ALL --check-prefix=O32 %s
 
 ; RUN: llc -mtriple=mips64-linux-gnu -relocation-model=static -mattr=-n64,+n32 < %s | FileCheck --check-prefix=ALL --check-prefix=N32 %s
 ; RUN: llc -mtriple=mips64el-linux-gnu -relocation-model=static -mattr=-n64,+n32 < %s | FileCheck --check-prefix=ALL --check-prefix=N32 %s
 
 ; RUN: llc -mtriple=mips64-linux-gnu -relocation-model=static -mattr=-n64,+n64 < %s | FileCheck --check-prefix=ALL --check-prefix=N64 %s
 ; RUN: llc -mtriple=mips64el-linux-gnu -relocation-model=static -mattr=-n64,+n64 < %s | FileCheck --check-prefix=ALL --check-prefix=N64 %s
 
+; RUN: llc -mtriple=mips-linux-gnu -relocation-model=static -mattr=+o32,+fp64 < %s | FileCheck --check-prefix=ALL --check-prefix=032FP64 %s
+; RUN: llc -mtriple=mipsel-linux-gnu -relocation-model=static -mattr=+o32,+fp64 < %s | FileCheck --check-prefix=ALL --check-prefix=032FP64 %s
+
 ; Test the float returns for all ABI's and byte orders as specified by
 ; section 5 of MD00305 (MIPS ABIs Described).
 
 ; We only test Linux because other OS's use different relocations and I don't
 ; know if this is correct.
 
 @float = global float zeroinitializer
 @double = global double zeroinitializer
entry:
         ret double %0
 }
 
 ; ALL-LABEL: retdouble:
 ; O32-DAG:           ldc1 $f0, %lo(double)([[R1:\$[0-9]+]])
 ; N32-DAG:           ldc1 $f0, %lo(double)([[R1:\$[0-9]+]])
 ; N64-DAG:           ld  [[R1:\$[0-9]+]], %got_disp(double)($1)
 ; N64-DAG:           ldc1 $f0, 0([[R1]])
+
+define { double, double } @retComplexDouble() #0 {
+  %retval = alloca { double, double }, align 8
+  %1 = load { double, double }* %retval
+  ret { double, double } %1
+}
+
+; ALL-LABEL: retComplexDouble:
+; 032FP64-DAG:      ldc1     $f0, 0($sp)
+; 032FP64-DAG:      ldc1     $f2, 8($sp)

llvm/trunk/test/CodeGen/Mips/cfi_offset.ll

+; RUN: llc -march=mips -mattr=+o32 < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32 < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EL
+; RUN: llc -march=mips -mattr=+o32,+fpxx < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32,+fpxx < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EL
+; RUN: llc -march=mips -mattr=+o32,+fp64 < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32,+fp64 < %s | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-EL
+
+@var = global double 0.0
+
+declare void @foo(...)
+
+define void @bar() {
+
+; CHECK-LABEL:  bar:
+
+; CHECK:  .cfi_def_cfa_offset 40
+; CHECK:  sdc1  $f22, 32($sp)
+; CHECK:  sdc1  $f20, 24($sp)
+; CHECK:  sw  $ra, 20($sp)
+; CHECK:  sw  $16, 16($sp)
+
+; CHECK-EB:  .cfi_offset 55, -8
+; CHECK-EB:  .cfi_offset 54, -4
+; CHECK-EB:  .cfi_offset 53, -16
+; CHECK-EB:  .cfi_offset 52, -12
+
+; CHECK-EL:  .cfi_offset 54, -8
+; CHECK-EL:  .cfi_offset 55, -4
+; CHECK-EL:  .cfi_offset 52, -16
+; CHECK-EL:  .cfi_offset 53, -12
+
+; CHECK:  .cfi_offset 31, -20
+; CHECK:  .cfi_offset 16, -24
+
+    %val1 = load volatile double* @var
+    %val2 = load volatile double* @var
+    call void (...)* @foo() nounwind
+    store volatile double %val1, double* @var
+    store volatile double %val2, double* @var
+    ret void
+}