This is an archive of the discontinued LLVM Phabricator instance.

[mips][ias] Implement N32 case for .cpsetup.
ClosedPublic

Authored by dsanders on Jun 8 2016, 6:37 AM.

Diff Detail

Event Timeline

dsanders updated this revision to Diff 60024.Jun 8 2016, 6:37 AM
dsanders retitled this revision from to [mips][ias] Implement N32 case for .cpsetup..
dsanders updated this object.
dsanders added a reviewer: sdardis.
dsanders added a subscriber: llvm-commits.
sdardis requested changes to this revision.Jun 8 2016, 9:31 AM
sdardis edited edge metadata.

This looks ok, but it acts unconditionally. In comparison GAS requires the -mno-shared to generate these style .cpsetup expansions as the resulting code cannot be put in a shared library.

This should only be enabled with a new command line flag -mno-shared.

lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp
1094

Comment here on that it's a specific GNU extension / optimization for N32, off by default as it cannot be used to generate libraries.

This revision now requires changes to proceed.Jun 8 2016, 9:31 AM

This looks ok, but it acts unconditionally. In comparison GAS requires the -mno-shared to generate these style .cpsetup expansions as the resulting code cannot be put in a shared library.

This should only be enabled with a new command line flag -mno-shared.

I'd like to come back to the -mshared implementation later if you're ok with that. This is partly because it's not the default case (at least not on our mips-mti-linux-gnu-gcc toolchain) and we don't have the relevant option yet, but it's mostly because we don't support %hi(%neg(%gp_rel(foo))) expressions properly for N32 at the moment (only one of the three relocs is emitted). Fixing that requires further changes to our MCFixups and the handling of this nested expression. This patch currently fixes just enough for me to fix the ELFCLASS bug (N32 should be ELFCLASS32 but is currently ELFCLASS64) in a way that allows it to have cpsetup.s check for a correct output instead of having to change it to another incorrect output.

Is that ok with you?

sdardis accepted this revision.Jun 14 2016, 2:02 AM
sdardis edited edge metadata.

Ok. LGTM in that case.

This revision is now accepted and ready to land.Jun 14 2016, 2:02 AM

Thanks. I'll clarify that it doesn't cover the -mshared case and why that part has been deferred in the commit message.

dsanders closed this revision.Jun 14 2016, 3:20 AM