Page MenuHomePhabricator

[mips] Add assembler support for the .cprestore directive.
ClosedPublic

Authored by dsanders on Nov 14 2014, 6:16 AM.

Details

Summary

This assembler directive is used in O32 PIC to restore the current function's $gp after executing JAL's. The $gp is first stored on the stack at a user-specified offset.
It has the following format: ".cprestore 8" (where 8 is the offset).

This fixes llvm.org/PR20967.

Diff Detail

Event Timeline

tomatabacu updated this revision to Diff 16208.Nov 14 2014, 6:16 AM
tomatabacu retitled this revision from to [mips] Add assembler support for the .cprestore directive..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added a reviewer: dsanders.
tomatabacu added a subscriber: Unknown Object (MLST).
emaste added a subscriber: emaste.Jan 28 2015, 8:20 AM
emaste added inline comments.Jan 28 2015, 1:19 PM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1758–1759

According to http://techpubs.sgi.com/library/dynaweb_docs/0530/SGI_Developer/books/Cplr_PTG/sgi_html/apa.html .cprestore should be a silent nop in non-PIC mode so that the same assembly source can be used in both PIC and non-PIC mode.

tomatabacu added inline comments.Jan 29 2015, 2:00 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1758–1759

I know what you mean, but I believe you've highlighted the wrong code.
The warning we were talking about on Bugzilla is for using .cprestore in non-PIC mode (MipsAsmParser.cpp at 3373-3374), but the warning you've highlighted here is for *not* using .cprestore *in* PIC mode.

tomatabacu added inline comments.Jan 29 2015, 2:02 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1758–1759

Forgot to mention that the warning highlighted here is also implemented in GAS.

tomatabacu updated this revision to Diff 18959.Jan 29 2015, 7:35 AM

Removed 2 warnings which were considered too strict (as discussed in llvm.org/PR20967).
Updated checks for the N32 and N64 ABIs in MipsTargetELFStreamer.
Updated tests because the microMIPS disassembler got fixed.
Rebased.

emaste added inline comments.Jan 29 2015, 8:22 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1758–1759

You're right, of course. I just wanted to make sure the comment was in the review and grabbed the wrong warning.

dsanders added inline comments.Feb 12 2015, 8:18 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
403

Just to be on the safe side, could you also initialize CpRestoreOffset in the constructor?

1559

Nit: if(!ExpandedJalSym)

1755–1789

I'm a bit confused by this code. Emitting a nop after a jal/jalr/jalrs makes sense to me, but I don't understand why the presence of the nop depends on inPicMode() and (!N32 && !N64).

I'm also not sure why the non-macro versions of jalr/jalrs (which set ExpandedJalSym to true) are causing createCpRestoreMemOp() to be called.

1759

This comment isn't quite right. It's emitting a nop if we are in '.set noreorder' mode.

1923–1924

We ought to factor out the nop emission at some point

Replied to an inline comment.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1923–1924

Done in http://reviews.llvm.org/D8320. I'd rather wait until that gets committed before I update this one, instead of juggling around with all these patches.

tomatabacu updated this revision to Diff 22091.Mar 17 2015, 7:30 AM

Addressed review comments.
Fixed duplicate emission of JALR when expanding a "JAL symbol".
Removed redundant test cases.
Added NOT checks.
Cleaned up tests.

Replied to inline comments.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
403

Done.

1559

Actually found a bug with this, so the "if" you commented about no longer exists, but I addressed the nit for the new code.

1755–1789

.cprestore causes an LW of $gp to be emitted after an expanded "JAL symbol" (which becomes a JALR) and it does so only if we're in PIC mode and using the O32 ABI. Between that JALR and the LW, we emit a NOP, which will have the same dependencies as .cprestore.

In response to your 2nd comment, AFAICT, ExpandedJalSym is true only if we did the PIC expansion for a "JAL symbol".

1759

Fixed.

1923–1924

Fully done.

dsanders added inline comments.Apr 10 2015, 1:54 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1755–1789

.cprestore causes an LW of $gp to be emitted after an expanded "JAL symbol" (which becomes a JALR) and it does so only if we're in PIC mode
and using the O32 ABI. Between that JALR and the LW, we emit a NOP, which will have the same dependencies as .cprestore.

Ah ok. So it's true when .cprestore has a visible effect on the output and false otherwise. That makes sense to me. In that case I'd suggest using inline functions to give this condition a more meaningful name (maybe shouldCprestoreModifyJal() or shouldRestoreGPAfterJal()?).

In response to your 2nd comment, AFAICT, ExpandedJalSym is true only if we did the PIC expansion for a "JAL symbol".

I see where my confusion is. jal is a macro that potentially expands to the jal instruction (or a sequence involving the jal instruction) and we're using the tablegen-erated assembly parsing for the jal instruction to parse the jal macro. The parsing gives us an MCInst which is the jal instruction but we expand it as if it were the jal macro to produce a jal instruction (or a sequence involving the jal instruction). Simple as that :-). It's a consequence of having the assembler and codegen in the same source which makes it difficult to separate the overlapping jal syntax. As confusing as it is, I can't think of a better way to go about it at the moment. I think we should comment on how this expansion works and how the line between instructions and macros are a bit blurry somewhere (the start of this if-statement?) and try to resolve the confusion later. Maybe, we can do something involving isCodeGenOnly and a assembler-only pseudo.

By the way, I've just noticed that we don't seem to be honouring '.set nomacro' properly. At least, I don't seem to be able to find any suitable guards near/in needsExpansion() and expandInstruction(). If we aren't honoring it then -fintegrated-as is likely to emit different objects for assembling via temporary assembly files and directly from codegen. Our codegen uses '.set nomacro' so we should fix this soon.

Rebased (for Sean Bruno's convenience).

tomatabacu planned changes to this revision.Jun 11 2015, 11:11 AM
dsanders edited edge metadata.Jun 12 2015, 5:55 AM

It's been a while since I reviewed this but I saw you'd rebased it again and took another look. I believe we had resolved almost all the issues and I was just waiting for a fairly minor change (see the shouldCprestoreModifyJal() comment).

tomatabacu updated this revision to Diff 28015.Jun 19 2015, 4:10 AM
tomatabacu edited edge metadata.

Outlined the (inPicMode() && !(isABI_N32() || isABI_N64())) condition into a separate function.
Switched to using !isInt<16>() in createCpRestoreMemOp() if condition.
Moved forbidModuleDirective() calls to the MipsTargetStreamer base class.
Removed redundant addition to the .set reorder NOP-creation condition.
Improved code comments to better explain why we need a NOP after the JALR in .set noreorder.

I believe this addresses all the remaining review comments for this patch.

tomatabacu updated this object.Jun 19 2015, 4:11 AM
dsanders accepted this revision.Jun 23 2015, 6:09 AM
dsanders edited edge metadata.

LGTM with a couple nits and a more concept-focused spelling for the new predicate.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
1761

This spelling says the same thing as the condition did. The intent was to name the predicate after the concept being tested rather than the mechanical values being tested.

1777–1779

Nit: Redundant braces.

2650–2653

Nit: Use ternary operator

This revision is now accepted and ready to land.Jun 23 2015, 6:09 AM
brooks added a subscriber: brooks.Jul 21 2015, 2:15 PM
seanbruno accepted this revision.Aug 16 2015, 4:29 PM
seanbruno added a reviewer: seanbruno.

Thanks for the work on this!

dsanders commandeered this revision.Aug 18 2015, 2:30 AM
dsanders edited reviewers, added: tomatabacu; removed: dsanders.

Thanks for triggering an email on this thread. It wasn't on my list of patches to commandeer when Toma finished his placement.

I'll fix the remaining nits and get this committed.

Seems to still apply (and FreeBSD needs it to build our mips/mips64 targets).

dsanders marked 16 inline comments as done.Sep 17 2015, 8:11 AM

Apologies for the delay on this. I had it ready to commit a few weeks ago but it seems I never actually committed it.

I'm re-building and re-testing now and will commit shortly assuming the tests pass.

dsanders closed this revision.Sep 17 2015, 9:10 AM

Committed in r247897