This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by jkolek on Jun 26 2014, 9:48 AM.

Details

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.

Diff Detail

Event Timeline

jkolek updated this revision to Diff 10888.Jun 26 2014, 9:48 AM
jkolek retitled this revision from to Emit two CFI offset directives per double precision SDC1/LDC1 instead of just one for FR=1 registers.
jkolek updated this object.
jkolek edited the test plan for this revision. (Show Details)
jkolek added reviewers: dsanders, vmedic.
jkolek added a subscriber: zoran.jovanovic.
jkolek updated this revision to Diff 10917.Jun 27 2014, 1:23 AM
vmedic added inline comments.Jun 27 2014, 1:29 AM
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 added inline comments.Jun 27 2014, 1:45 AM
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 updated this revision to Diff 10921.Jun 27 2014, 3:32 AM

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

jkolek updated this revision to Diff 11051.Jul 3 2014, 5:45 AM

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

dsanders added inline comments.Jul 8 2014, 1:29 PM
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
17–20

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 added inline comments.Jul 9 2014, 2:24 AM
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 added inline comments.Jul 9 2014, 3:23 AM
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 updated this revision to Diff 11278.Jul 10 2014, 8:44 AM

Fixed error in comment, test for complex values added.

dsanders edited edge metadata.Jul 10 2014, 9:30 AM

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 updated this revision to Diff 11280.Jul 10 2014, 10:36 AM
jkolek edited edge metadata.

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

dsanders accepted this revision.Jul 10 2014, 12:21 PM
dsanders edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jul 10 2014, 12:21 PM

Please fix the retComlpexDouble typo though.

jkolek updated this revision to Diff 11289.Jul 10 2014, 12:42 PM
jkolek edited edge metadata.

Typo fixed.

Thanks. LGTM

jkolek closed this revision.Jul 10 2014, 3:32 PM
jkolek updated this revision to Diff 11297.

Closed by commit rL212769 (authored by zjovanovic).

jkolek added a subscriber: Unknown Object (MLST).Nov 18 2014, 5:54 AM