This is an archive of the discontinued LLVM Phabricator instance.

Try to handle 'dla' in PIC mode for N64.
ClosedPublic

Authored by bsdjhb on Jun 6 2017, 11:02 AM.

Details

Summary

[MIPS] Handle PIC load address macro instructions in N64.

In particular, use CALL16 (similar to O32) for address loads into T9 for certain
cases. Otherwise use a %got_disp relocation to load the address of a symbol.
Small offsets (small enough to fit in a 16-bit signed immediate) can be used and
are added to the symbol address after it is loaded from the GOT. Larger offsets
are currently unsupported and result in an error from the assembler.

Event Timeline

bsdjhb created this revision.Jun 6 2017, 11:02 AM
bsdjhb added inline comments.Jun 6 2017, 11:03 AM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2955

This comment is now stale and needs updating, I'm just not quite sure what the correct new comment is.

sdardis edited edge metadata.Jun 6 2017, 11:43 AM

I do not yet have test cases written (I'm not familiar with writing those for llvm),

If you look in test/MC/Mips/macro-la-pic.s , you'll find the test for the O32 'la' macro.

Copy that file to test/MC/Mips/macro-dla-pic.s, and modify the RUN: lines that mention -mtriple=mips-unknown-linux to -mtriple=mips64-unknown-linux and modify the -mcpu=mips32rX to -mcpu=mips64r6.

You'll need change the CHECK: lines to account for the change in instructions, as the encoding will be different due to using 'ld' instead of 'lw'. See http://llvm.org/docs/CommandGuide/FileCheck.html for more details on how FileCheck works.

You can run a single test by invoking llvm-lit from the build directory. e.g:

./bin/llvm-lit test/MC/Mips/macro-la-pic.s -a

Will run just that test and be verbose (the '-a') about it. You'll also see the commands use to run the test and use that to find the encodings for the different instructions.

I'll take a longer look at this tomorrow.

Thanks for doing this,
Simon

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

The '&& isGP64bit()' is unnecessary, ArePtrs64bit() should be sufficient to cover N64.

2955

I'm not wholly sure right now myself, but if you grab a recent copy of binutils and experiment (you'll want to see what GCC produces for PIC code as a reference), you'll get something.

2966–2968

XXX -> FIXME. LLVM uses "FIXME"s over XXX or ???.

sdardis requested changes to this revision.Jun 7 2017, 3:01 AM

Can you split out the change of choosing CALL16 vs GOT for O32 at least? It's not wholly relevant to this patch and IIRC correctly, there may be deficiency in the assembly parser regarding forward declared local symbols. I have a patch floating around that addresses that.

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

Can you add "FIXME: These expansions do not respect -mxgot."

2867–2872

This change should be submitted seperately.

2966–2968

N64 and N32 both use GOT_DISP, so the bottom FIXME can be removed.

2971

While the O32 ABI produces a R_MIPS_LO16 for symbols that are temporary or located in the current section, N64/ N32 produce a R_MIPS_GOT16 only.

2972–2976

This hunk and the addition when LoExpr != nullptr is incorrect when the constant is < -0x8000 || >0x7fff as the symbol offset is truncated. Furthermore, GAS indicates that the offset for a symbol is 32 bits are most.

This revision now requires changes to proceed.Jun 7 2017, 3:01 AM
bsdjhb marked 4 inline comments as done.Jun 7 2017, 10:29 AM
bsdjhb added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2972–2976

To be clear, does this mean that LoExpr should just be dropped entirely here? In the specific use cases I've seen this with FreeBSD there isn't an offset so I've not exercised this case (and am not sure what the right thing to do is)

bsdjhb updated this revision to Diff 101786.Jun 7 2017, 11:31 AM
bsdjhb edited edge metadata.
  • Rebase on top of D33999
  • A few fixes suggested by Simon Dardis.
  • Add initial tests.
  • Start on updating the comments, though I don't think this is quite right still.
bsdjhb updated this revision to Diff 103541.Jun 22 2017, 2:28 AM
  • Don't include the symbol offset in the %got_disp relocation.
  • Add missing daddui's for symbol offsets to the test.
  • Update comments and punt on large offsets for now.
bsdjhb marked an inline comment as done.Jun 22 2017, 2:30 AM
sdardis requested changes to this revision.Jun 27 2017, 3:21 AM

Some small changes requested, but otherwise this looks good.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2866–2867

Can you remove that change?

The expansion for 'la' and 'dla' in the PIC case are very similar. I'd like both chunks of code to be as similar as possible so that the refactoring is as easy as possible.

2942–2947

Formatting. See my comment on D33999.

2962–2963

Formatting. This should be:

const MipsMCExpr *GotExpr = MipsMCExpr::create(MipsMCExpr::MEK_GOT_DISP,
                                               Res.getSymA(), getContext());
2971

FIXME: Offsets greater than 16 bits are not yet implemented.
FIXME: The correct range is a 32 bit sign-extended number.

2972–2976

The handling of of a constant and a symbol here is correct for the case of a sign extended 16 bit number.

2974

"macro instruction uses large offset, which is not currently supported", also add test for this error.

See test/MC/Mips/macro-la-bad.s for an example of how to write a test that tests an error.

test/MC/Mips/macro-dla-pic.s
1 ↗(On Diff #103541)

The cpu here should be mips3.

This revision now requires changes to proceed.Jun 27 2017, 3:21 AM
bsdjhb marked 6 inline comments as done.Jun 27 2017, 4:26 PM
bsdjhb added inline comments.
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2866–2867

Do you want me to check for both T9 and T9_64 in the new 64-bit block as well?

sdardis added inline comments.Jun 27 2017, 5:08 PM
lib/Target/Mips/AsmParser/MipsAsmParser.cpp
2866–2867

Please. I'll sit down and refactor this code after it's been committed.

bsdjhb updated this revision to Diff 104398.Jun 28 2017, 6:04 AM
bsdjhb edited edge metadata.
bsdjhb marked an inline comment as done.
  • Revert change to remove T9_64 from O32 CALL16 condition.
  • Formatting and other fixes from Simon.
  • Add tests for errors for large symbol offsets with dla.
  • Check for both T9's in the 64-bit case.
bsdjhb updated this revision to Diff 104399.Jun 28 2017, 6:06 AM
  • Wrap long line per clang-format.
bsdjhb marked an inline comment as done.Jun 28 2017, 6:08 AM
bsdjhb added inline comments.
test/MC/Mips/macro-dla-pic.s
3 ↗(On Diff #104399)

Not sure if this comment is really accurate. It was a cut and paste from macro-la-pic.s.

sdardis added inline comments.Jun 28 2017, 6:30 AM
test/MC/Mips/macro-dla-pic.s
3 ↗(On Diff #104399)

Yes, it incorrect. Instead of erroring out, a static relocation sequence is written out. Also we don't warn + transform to 'la' instead like GAS does.

bsdjhb updated this revision to Diff 104436.Jun 28 2017, 9:10 AM
  • Drop N32 comment from dla-pic test.
sdardis accepted this revision.Jun 29 2017, 10:45 AM

LGTM with a summary update.

This revision is now accepted and ready to land.Jun 29 2017, 10:45 AM
bsdjhb edited the summary of this revision. (Show Details)Jun 29 2017, 10:55 AM

I've taken a stab at a new summary. Simon, can you refine that further and commit this change as I do not have commit access?

This revision was automatically updated to reflect the committed changes.