This is an archive of the discontinued LLVM Phabricator instance.

[mips] Add the following MIPS options that control gp-relative addressing of small data items: -mgpopt, -mlocal-sdata, -mextern-sdata.
ClosedPublic

Authored by sstankovic on Aug 14 2014, 4:28 AM.

Details

Summary

[mips] Add the following MIPS options that control gp-relative addressing of small data items: -mgpopt, -mlocal-sdata, -mextern-sdata. Implement gp-relative addressing for constants.

Diff Detail

Repository
rL LLVM

Event Timeline

sstankovic updated this revision to Diff 12496.Aug 14 2014, 4:28 AM
sstankovic retitled this revision from to [mips] Add the following MIPS options that control gp-relative addressing of small data items: -mgpopt, -mlocal-sdata, -mextern-sdata..
sstankovic updated this object.
sstankovic edited the test plan for this revision. (Show Details)
sstankovic added reviewers: dsanders, mseaborn.
sstankovic added a project: deleted.
sstankovic added a subscriber: Unknown Object (MLST).
petarj added a subscriber: petarj.Aug 14 2014, 5:54 AM

Some notes about the patch. Patch is implemented so that if -mgpopt is not specified, then default value depends on the target - it is false for Linux and true for bare metal. gcc does it differently - -mgpopt is always true by default, and -G option actually controls gp-relative addressing - it is 0 by default on Linux, and 8 for bare metal. (On targets where -mabicalls is true by default (Linux), -G is 0 by default, and on targets where is -mabicalls is false by default (bare metal) -G is 8.). gcc does not generate gp-relative addressing for -mabicalls, and this is also implemented in the patch.

So in gcc, to use gp-relative addressing:

mips-linux-gnu-gcc -mno-abicalls -G 8 hello.c -o hello (Linux)
mips-sde-elf-gcc hello.c -o hello (bare metal)

The equivalent to -G is named -mips-ssection-threshold in llc (added by some previous commit), and the patch doesn't change that.

dsanders edited edge metadata.Aug 19 2014, 5:32 AM

I'm not very familiar with the bare-metal side of things at the moment so I'll need to do some reading before I can review this patch properly. I wanted to ask a question about the defaults though.

In D4903#6, @sstankovic wrote:

Some notes about the patch. Patch is implemented so that if -mgpopt is not specified, then default value depends on the target - it is false for Linux and true for bare metal. gcc does it differently - -mgpopt is always true by default, and -G option actually controls gp-relative addressing - it is 0 by default on Linux, and 8 for bare metal. (On targets where -mabicalls is true by default (Linux), -G is 0 by default, and on targets where is -mabicalls is false by default (bare metal) -G is 8.). gcc does not generate gp-relative addressing for -mabicalls, and this is also implemented in the patch.

So in gcc, to use gp-relative addressing:

mips-linux-gnu-gcc -mno-abicalls -G 8 hello.c -o hello (Linux)
mips-sde-elf-gcc hello.c -o hello (bare metal)

The equivalent to -G is named -mips-ssection-threshold in llc (added by some previous commit), and the patch doesn't change that.

I think I may be missing something here. If llvm's -mips-ssection-threshold is a direct equivalent to gcc's -G then why not treat the defaults for -mgpopt/-mips-ssection-threshold in llvm the same way as the defaults for -mgpopt/-G in gcc?

sstankovic edited edge metadata.

Fixed the bug in function MipsTargetObjectFile::getSectionForConstant by using call to IsConstantInSmallSection instead of IsInSmallSection. Changed the patch to handle defaults for -mgpopt/-mips-ssection-threshold the same way as the defaults for -mgpopt/-G in gcc.

I haven't read through the test case yet but here's my comments on the code.

lib/Target/Mips/MipsSubtarget.cpp
183–186 ↗(On Diff #12672)

This is unnecessary. ParseSubtargetFeatures() has already checked the feature bit and set NoABICalls to true, and Linux/NaCl get their behaviour from -mabicalls being the default (which can be overridden with -mno-abicalls).

I think you may be trying to make llc behave the same way as the clang driver. Is this correct? If so, I'm not sure this is a sensible direction. We'd end up with two implementations of the driver, one in the backend and one in the frontend. It would be preferable to pass the appropriate options in the testcases.

194–209 ↗(On Diff #12672)

This seems more complicated than necessary. I don't think we need to test for Linux and NaCl since their behaviour is determined by the fact that they enable -mabicalls by default (which in turn disables $gp relative accesses). Also, why not have the default set to 8 and force the value to zero when !GPOpt or NoAbiCalls?

lib/Target/Mips/MipsTargetObjectFile.cpp
53 ↗(On Diff #12672)

Could you explain why zero-sized objects are excluded?

61 ↗(On Diff #12672)

Can you fix the indentation while you're here?

65 ↗(On Diff #12672)

Are static globals allowed in the small section?

76–77 ↗(On Diff #12672)

Style nit: I believe clang-format would put the && on the previous line.

84 ↗(On Diff #12672)

Style nit: Indentation

102 ↗(On Diff #12672)

Style nit: || should be on previous line

126–142 ↗(On Diff #12672)

Just a question: Are constants better off in .rodata where they are protected from accidental writes or .sdata?

sstankovic updated this revision to Diff 14648.Oct 9 2014, 4:42 AM
sstankovic set the repository for this revision to rL LLVM.
sstankovic removed a project: deleted.

Patch is updated. I'll post the comments very soon.

sstankovic added inline comments.Oct 9 2014, 10:14 AM
lib/Target/Mips/MipsSubtarget.cpp
183–186 ↗(On Diff #12672)

I added this to support non-Linux and non-NaCl targets (like mipsel-sde-elf), because clang driver currently doesn't support it. But I agree this is driver's job, so I removed it (and the similair check below).

194–209 ↗(On Diff #12672)

Done. (I actually restored the logic from initial implementation of the patch, and simplified it by removing the check for Linux and NaCl).

lib/Target/Mips/MipsTargetObjectFile.cpp
53 ↗(On Diff #12672)

I don't know the reason for it. I followed gcc - there is a comment in file gcc/config/mips/mips.c, in function mips_in_small_data_p:

/* We have traditionally not treated zero-sized objects as small data,
   so this is now effectively part of the ABI.  */
return size > 0 && size <= mips_small_data_threshold;
65 ↗(On Diff #12672)

Do you mean globals defined in the same file? If so, they are.

126–142 ↗(On Diff #12672)

I suppose they are. There is actually another MIPS-specific gcc option that can control this, -membedded-data (off by default), that if enabled places constants in .rodata. It also applies to read-only variables.

Thanks. There's just one thing to clarify.

lib/Target/Mips/MipsTargetObjectFile.cpp
53 ↗(On Diff #12672)

Thanks. We should comment that it's important for the ABI so that future readers don't try to remove it.

65 ↗(On Diff #12672)

I mean globals defined in the same file that aren't available/visible to other objects.

sstankovic added inline comments.Oct 10 2014, 4:05 AM
lib/Target/Mips/MipsTargetObjectFile.cpp
65 ↗(On Diff #12672)

They are allowed, just like the globals defined in the same file that are visible to other objects. This two cases are handled in .ll file by "internal" and "data" tests, respectively. The difference between them is that gp_rel for static globals can be disabled using -mlocal-sdata=0.

sstankovic updated this revision to Diff 14712.Oct 10 2014, 4:18 AM

Added comment why zero-sized objects are not treated as small data.

dsanders accepted this revision.Nov 4 2014, 1:43 AM
dsanders edited edge metadata.

LGTM. Sorry for the delay in getting back to this

This revision is now accepted and ready to land.Nov 4 2014, 1:43 AM
sstankovic closed this revision.Nov 6 2014, 5:31 AM
sstankovic updated this revision to Diff 15860.

Closed by commit rL221450 (authored by @sstankovic).