[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.
Details
Diff Detail
Event Timeline
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'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.
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?
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 | ||
---|---|---|
181–182 | 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. | |
182–188 | 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 | ||
61 | Could you explain why zero-sized objects are excluded? | |
63–69 | Can you fix the indentation while you're here? | |
70 | Are static globals allowed in the small section? | |
81–82 | Style nit: I believe clang-format would put the && on the previous line. | |
89 | Style nit: Indentation | |
108 | Style nit: || should be on previous line | |
132–148 | Just a question: Are constants better off in .rodata where they are protected from accidental writes or .sdata? |
lib/Target/Mips/MipsSubtarget.cpp | ||
---|---|---|
181–182 | 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). | |
182–188 | 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 | ||
61 | 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; | |
70 | Do you mean globals defined in the same file? If so, they are. | |
132–148 | 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. |
lib/Target/Mips/MipsTargetObjectFile.cpp | ||
---|---|---|
70 | 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. |
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.