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

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

345

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

351

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

[dsanders]

lib/Target/Mips/MipsSEFrameLowering.cpp

342

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

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

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

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

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

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

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).

lib/Target/Mips/MipsCallingConv.td

 
   // f64 arguments are returned in D0_64 and D1_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]>>>
 ]>;
 

 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,
+                       (sequence "S%u", 7, 0))>;
 
 def CSR_N32 : CalleeSavedRegs<(add D20_64, D22_64, D24_64, D26_64, D28_64,
                                    D30_64, RA_64, FP_64, GP_64,

lib/Target/Mips/MipsSEFrameLowering.cpp

             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(

test/CodeGen/Mips/cfi_offset.ll

+; RUN: llc -march=mips -mattr=+o32 < %s | FileCheck %s --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32 < %s | FileCheck %s --check-prefix=CHECK-EL
+; RUN: llc -march=mips -mattr=+o32,+fpxx < %s | FileCheck %s --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32,+fpxx < %s | FileCheck %s --check-prefix=CHECK-EL
+; RUN: llc -march=mips -mattr=+o32,+fp64 < %s | FileCheck %s --check-prefix=CHECK-EB
+; RUN: llc -march=mipsel -mattr=+o32,+fp64 < %s | FileCheck %s --check-prefix=CHECK-EL
+
+@var = global double 0.0
+
+declare void @foo(...)
+
+define void @bar() {
+
+; CHECK-LABEL:  bar:
+
+; CHECK:  .cfi_def_cfa_offset 32
+; 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, -12
+; CHECK:  .cfi_offset 16, -16
+
+    %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
+}