Don't access items in .sdata section using gp-relative addressing when target is NaCl.

[sstankovic]

Summary

[mips] Don't access items in .sdata section using gp-relative addressing when target is NaCl.

[mseaborn]

Can you provide some more context in the commit message, please? Why don't you want to use gp-relative addressing when targeting NaCl? Is this necessary or just desirable?

[sstankovic]

Here is some explanation for it, but I'm not sure whether this should be in the commit message. This is also not supported in Linux, but I don't know the reason for it. But even if we enable gp-relative addressing in NaCl, it wouldn't work because MIPS Gold linker currently doesn't support it. For gp-relative addressing to work, linker needs to emit together all the sections that are accessed using gp-relative addressing (.got section first, followed by various small-data sections (.sdata, .sbss, ...)). This is currently not implemented in MIPS Gold. (In GNU ld, this section ordering is specified in ld's internal linker script, but Gold doesn't have internal linker script.)

[sstankovic]

Presumably the reason that UseSmallSection is disabled for Linux is that LLVM is assuming that it might be being used with Gold?

It isn't related to Gold because apart from PNaCl, MIPS Gold is not used anywhere else; MIPS Gold isn't even committed to the FSF binutils - the patch is still in review on binutils mailing list.

I suppose that the reason why it's disabled in Linux is because gp-relative addressing can only address 64K area (MIPS has 16-bit load/store offsets). That's probably why it's never used for PIC code (both Linux and non-Linux), because in PIC code .got section is also accessed using gp-relative addressing, so that leaves even less space for data items to be accessed using gp-relative addressing.

If you don't explain this in the commit message or in the code, how would someone reading the
source code know the reason for making UseSmallSection conditional on !isTargetNaCl()? How
would someone maintaining the code know whether the !isTargetNaCl() check could be removed?

I think you should add a comment explaining this in the code.

When I said that I was not sure whether this should be in the commit message I was referring to Gold (I think we should not mention Gold in LLVM code or commit message).

I updated the patch with the comment that in NaCl we disable gp-relative addressing because it can address 64K area only.

[sstankovic]

Would it make sense to turn off use of sdata by default, and have it only enabled by a compiler flag or llc flag? That would be cleaner and is not very difficult to do. It should come with a warning like "this is an optimisation that might cause moderate-sized programs to fail to link".

This is what Mips gcc for Linux does - gp-relative addressing is not enabled by default, but it can be requested at command line (gcc doesn't issue a warning though). I also tested a bare-metal Mips gcc toolchain, and it has different behavior - gp-relative addressing is enabled by default. I'll ask Daniel Sanders (LLVM code owner for MIPS backend) whether we should add a command line flag for this.

BTW, have you looked at the GlobalMerge pass (lib/Transforms/Scalar/GlobalMerge.cpp)? That is used on ARM and gives a similar kind of optimisation to the MIPS sdata section. It will reuse the computation of a base address within the data segment for multiple variables, rather than computing a data segment address for each variable that's referenced. GlobalMerge is a more robust way of doing this optimisation than sdata, though it only works when the referenced variables are within the same compilation unit being compiled by llc.

I enabled this pass for Mips and experimented with it by running some tests. When the pass changes the code, it usually does create smaller code. But there are also cases when the pass actually produces larger code. I'll do more investigation on this.

[sstankovic]

Implemented in D4903.

lib/Target/Mips/MipsSubtarget.cpp

MipsSubtarget::MipsSubtarget(const std::string &TT, const std::string &CPU,
 
   // Is the target system Linux ?
   if (TT.find("linux") == std::string::npos)
     IsLinux = false;
 
   // Set UseSmallSection.
   // TODO: Investigate the IsLinux check. I suspect it's really checking for
   //       bare-metal.
-  UseSmallSection = !IsLinux && (RM == Reloc::Static);
+  UseSmallSection = !IsLinux && !isTargetNaCl() && (RM == Reloc::Static);
 }
 
 bool
 MipsSubtarget::enablePostRAScheduler(CodeGenOpt::Level OptLevel,
                                     TargetSubtargetInfo::AntiDepBreakMode &Mode,
                                      RegClassVector &CriticalPathRCs) const {
   Mode = TargetSubtargetInfo::ANTIDEP_NONE;
   CriticalPathRCs.clear();

test/CodeGen/Mips/nacl-small-section.ll

+; RUN: llc -filetype=asm -mtriple=mipsel-none-nacl -relocation-model=static \
+; RUN:     < %s | FileCheck %s
+
+
+@x = global i32 1, align 4
+
+define i32 @test1() {
+  %1 = load i32* @x, align 4
+  ret i32 %1
+
+
+; CHECK-LABEL:       test1
+
+; Check that in NaCl the items in .sdata secton are not accessed using
+; gp-relative addressing.
+
+; CHECK-NOT:         %gp_rel(x)($gp)
+}