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

[sstankovic]

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.

[sstankovic]

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]

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]

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.

[dsanders]

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]

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

[sstankovic]

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.

[dsanders]

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]

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]

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

[dsanders]

LGTM. Sorry for the delay in getting back to this

[sstankovic]

Closed by commit rL221450 (authored by @sstankovic).

llvm/trunk/lib/Target/Mips/MipsISelLowering.h

SDValue getAddrNonPIC(NodeTy *N, EVT Ty, SelectionDAG &DAG) const {
       SDLoc DL(N);
       SDValue Hi = getTargetNode(N, Ty, DAG, MipsII::MO_ABS_HI);
       SDValue Lo = getTargetNode(N, Ty, DAG, MipsII::MO_ABS_LO);
       return DAG.getNode(ISD::ADD, DL, Ty,
                          DAG.getNode(MipsISD::Hi, DL, Ty, Hi),
                          DAG.getNode(MipsISD::Lo, DL, Ty, Lo));
     }
 
+    // This method creates the following nodes, which are necessary for
+    // computing a symbol's address using gp-relative addressing:
+    //
+    // (add $gp, %gp_rel(sym))
+    template<class NodeTy>
+    SDValue getAddrGPRel(NodeTy *N, EVT Ty, SelectionDAG &DAG) const {
+      SDLoc DL(N);
+      assert(Ty == MVT::i32);
+      SDValue GPRel = getTargetNode(N, Ty, DAG, MipsII::MO_GPREL);
+      return DAG.getNode(ISD::ADD, DL, Ty,
+                         DAG.getRegister(Mips::GP, Ty),
+                         DAG.getNode(MipsISD::GPRel, DL, DAG.getVTList(Ty),
+                                     GPRel));
+    }
+
     /// This function fills Ops, which is the list of operands that will later
     /// be used when a function call node is created. It also generates
     /// copyToReg nodes to set up argument registers.
     virtual void
     getOpndList(SmallVectorImpl<SDValue> &Ops,
                 std::deque< std::pair<unsigned, SDValue> > &RegsToPass,
                 bool IsPICCall, bool GlobalOrExternal, bool InternalLinkage,
                 bool IsCallReloc, CallLoweringInfo &CLI, SDValue Callee,

llvm/trunk/lib/Target/Mips/MipsISelLowering.cpp

SDValue MipsTargetLowering::lowerSETCC(SDValue Op, SelectionDAG &DAG) const {
   SDValue True  = DAG.getConstant(1, MVT::i32);
   SDValue False = DAG.getConstant(0, MVT::i32);
 
   return createCMovFP(DAG, Cond, True, False, SDLoc(Op));
 }
 
 SDValue MipsTargetLowering::lowerGlobalAddress(SDValue Op,
                                                SelectionDAG &DAG) const {
-  // FIXME there isn't actually debug info here
-  SDLoc DL(Op);
   EVT Ty = Op.getValueType();
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
   const GlobalValue *GV = N->getGlobal();
 
   if (getTargetMachine().getRelocationModel() != Reloc::PIC_ &&
       !Subtarget.isABI_N64()) {
     const MipsTargetObjectFile &TLOF =
       (const MipsTargetObjectFile&)getObjFileLowering();
 
+    if (TLOF.IsGlobalInSmallSection(GV, getTargetMachine()))
       // %gp_rel relocation
-    if (TLOF.IsGlobalInSmallSection(GV, getTargetMachine())) {
-      SDValue GA = DAG.getTargetGlobalAddress(GV, DL, MVT::i32, 0,
-                                              MipsII::MO_GPREL);
-      SDValue GPRelNode = DAG.getNode(MipsISD::GPRel, DL,
-                                      DAG.getVTList(MVT::i32), GA);
-      SDValue GPReg = DAG.getRegister(Mips::GP, MVT::i32);
-      return DAG.getNode(ISD::ADD, DL, MVT::i32, GPReg, GPRelNode);
-    }
+      return getAddrGPRel(N, Ty, DAG);
 
     // %hi/%lo relocation
     return getAddrNonPIC(N, Ty, DAG);
   }
 
   if (GV->hasInternalLinkage() || (GV->hasLocalLinkage() && !isa<Function>(GV)))
     return getAddrLocal(N, Ty, DAG,
                         Subtarget.isABI_N32() || Subtarget.isABI_N64());
lowerJumpTable(SDValue Op, SelectionDAG &DAG) const
 
   return getAddrLocal(N, Ty, DAG,
                       Subtarget.isABI_N32() || Subtarget.isABI_N64());
 }
 
 SDValue MipsTargetLowering::
 lowerConstantPool(SDValue Op, SelectionDAG &DAG) const
 {
-  // gp_rel relocation
-  // FIXME: we should reference the constant pool using small data sections,
-  // but the asm printer currently doesn't support this feature without
-  // hacking it. This feature should come soon so we can uncomment the
-  // stuff below.
-  //if (IsInSmallSection(C->getType())) {
-  //  SDValue GPRelNode = DAG.getNode(MipsISD::GPRel, MVT::i32, CP);
-  //  SDValue GOT = DAG.getGLOBAL_OFFSET_TABLE(MVT::i32);
-  //  ResNode = DAG.getNode(ISD::ADD, MVT::i32, GOT, GPRelNode);
   ConstantPoolSDNode *N = cast<ConstantPoolSDNode>(Op);
   EVT Ty = Op.getValueType();
 
   if (getTargetMachine().getRelocationModel() != Reloc::PIC_ &&
-      !Subtarget.isABI_N64())
+      !Subtarget.isABI_N64()) {
+    const MipsTargetObjectFile &TLOF =
+      (const MipsTargetObjectFile&)getObjFileLowering();
+
+    if (TLOF.IsConstantInSmallSection(N->getConstVal(), getTargetMachine()))
+      // %gp_rel relocation
+      return getAddrGPRel(N, Ty, DAG);
+
     return getAddrNonPIC(N, Ty, DAG);
+  }
 
   return getAddrLocal(N, Ty, DAG,
                       Subtarget.isABI_N32() || Subtarget.isABI_N64());
 }
 
 SDValue MipsTargetLowering::lowerVASTART(SDValue Op, SelectionDAG &DAG) const {
   MachineFunction &MF = DAG.getMachineFunction();
   MipsFunctionInfo *FuncInfo = MF.getInfo<MipsFunctionInfo>();

llvm/trunk/lib/Target/Mips/MipsSubtarget.cpp

Mips16HardFloat("mips16-hard-float", cl::NotHidden,
                 cl::init(false));
 
 static cl::opt<bool>
 Mips16ConstantIslands(
   "mips16-constant-islands", cl::NotHidden,
   cl::desc("MIPS: mips16 constant islands enable."),
   cl::init(true));
 
+static cl::opt<bool>
+GPOpt("mgpopt", cl::Hidden,
+      cl::desc("MIPS: Enable gp-relative addressing of small data items"));
+
 /// Select the Mips CPU for the given triple and cpu name.
 /// FIXME: Merge with the copy in MipsMCTargetDesc.cpp
 static StringRef selectMipsCPU(Triple TT, StringRef CPU) {
   if (CPU.empty() || CPU == "generic") {
     if (TT.getArch() == Triple::mips || TT.getArch() == Triple::mipsel)
       CPU = "mips32";
     else
       CPU = "mips64";
if (hasMips32r6()) {
     if (hasDSP())
       report_fatal_error(ISA + " is not compatible with the DSP ASE", false);
   }
 
   // Is the target system Linux ?
   if (TT.find("linux") == std::string::npos)
     IsLinux = false;
 
+  if (NoABICalls && TM->getRelocationModel() == Reloc::PIC_)
+    report_fatal_error("position-independent code requires '-mabicalls'");
+
   // Set UseSmallSection.
-  // TODO: Investigate the IsLinux check. I suspect it's really checking for
-  //       bare-metal.
-  UseSmallSection = !IsLinux && (TM->getRelocationModel() == Reloc::Static);
+  UseSmallSection = GPOpt;
+  if (!NoABICalls && GPOpt) {
+    errs() << "warning: cannot use small-data accesses for '-mabicalls'"
+           << "\n";
+    UseSmallSection = false;
+  }
 }
 
 /// This overrides the PostRAScheduler bit in the SchedModel for any CPU.
 bool MipsSubtarget::enablePostMachineScheduler() const { return true; }
 
 void MipsSubtarget::getCriticalPathRCs(RegClassVector &CriticalPathRCs) const {
   CriticalPathRCs.clear();
   CriticalPathRCs.push_back(isGP64bit() ?

llvm/trunk/lib/Target/Mips/MipsTargetObjectFile.h

 
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 
 namespace llvm {
 
   class MipsTargetObjectFile : public TargetLoweringObjectFileELF {
     const MCSection *SmallDataSection;
     const MCSection *SmallBSSSection;
+    const TargetMachine *TM;
   public:
 
     void Initialize(MCContext &Ctx, const TargetMachine &TM) override;
 
-
-    /// IsGlobalInSmallSection - Return true if this global address should be
-    /// placed into small data/bss section.
-    bool IsGlobalInSmallSection(const GlobalValue *GV,
-                                const TargetMachine &TM, SectionKind Kind)const;
+    /// Return true if this global address should be placed into small data/bss
+    /// section.
+    bool IsGlobalInSmallSection(const GlobalValue *GV, const TargetMachine &TM,
+                                SectionKind Kind) const;
     bool IsGlobalInSmallSection(const GlobalValue *GV,
                                 const TargetMachine &TM) const;
+    bool IsGlobalInSmallSectionImpl(const GlobalValue *GV,
+                                    const TargetMachine &TM) const;
 
     const MCSection *SelectSectionForGlobal(const GlobalValue *GV,
                                         SectionKind Kind, Mangler &Mang,
                                         const TargetMachine &TM) const override;
+
+    /// Return true if this constant should be placed into small data section.
+    bool IsConstantInSmallSection(const Constant *CN,
+                                  const TargetMachine &TM) const;
+
+    const MCSection *getSectionForConstant(SectionKind Kind,
+                                           const Constant *C) const override;
   };
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Target/Mips/MipsTargetObjectFile.cpp

 #include "llvm/Target/TargetMachine.h"
 using namespace llvm;
 
 static cl::opt<unsigned>
 SSThreshold("mips-ssection-threshold", cl::Hidden,
             cl::desc("Small data and bss section threshold size (default=8)"),
             cl::init(8));
 
+static cl::opt<bool>
+LocalSData("mlocal-sdata", cl::Hidden,
+           cl::desc("MIPS: Use gp_rel for object-local data."),
+           cl::init(true));
+
+static cl::opt<bool>
+ExternSData("mextern-sdata", cl::Hidden,
+            cl::desc("MIPS: Use gp_rel for data that is not defined by the "
+                     "current object."),
+            cl::init(true));
+
 void MipsTargetObjectFile::Initialize(MCContext &Ctx, const TargetMachine &TM){
   TargetLoweringObjectFileELF::Initialize(Ctx, TM);
   InitializeELF(TM.Options.UseInitArray);
 
   SmallDataSection =
     getContext().getELFSection(".sdata", ELF::SHT_PROGBITS,
                                ELF::SHF_WRITE |ELF::SHF_ALLOC,
                                SectionKind::getDataRel());
 
   SmallBSSSection =
     getContext().getELFSection(".sbss", ELF::SHT_NOBITS,
                                ELF::SHF_WRITE |ELF::SHF_ALLOC,
                                SectionKind::getBSS());
+  this->TM = &TM;
 }
 
 // A address must be loaded from a small section if its size is less than the
 // small section size threshold. Data in this section must be addressed using
 // gp_rel operator.
 static bool IsInSmallSection(uint64_t Size) {
+  // gcc has traditionally not treated zero-sized objects as small data, so this
+  // is effectively part of the ABI.
   return Size > 0 && Size <= SSThreshold;
 }
 
-bool MipsTargetObjectFile::IsGlobalInSmallSection(const GlobalValue *GV,
-                                                const TargetMachine &TM) const {
+/// Return true if this global address should be placed into small data/bss
+/// section.
+bool MipsTargetObjectFile::
+IsGlobalInSmallSection(const GlobalValue *GV, const TargetMachine &TM) const {
+  // We first check the case where global is a declaration, because finding
+  // section kind using getKindForGlobal() is only allowed for global
+  // definitions.
   if (GV->isDeclaration() || GV->hasAvailableExternallyLinkage())
-    return false;
+    return IsGlobalInSmallSectionImpl(GV, TM);
 
   return IsGlobalInSmallSection(GV, TM, getKindForGlobal(GV, TM));
 }
 
-/// IsGlobalInSmallSection - Return true if this global address should be
-/// placed into small data/bss section.
+/// Return true if this global address should be placed into small data/bss
+/// section.
 bool MipsTargetObjectFile::
 IsGlobalInSmallSection(const GlobalValue *GV, const TargetMachine &TM,
                        SectionKind Kind) const {
+  return (IsGlobalInSmallSectionImpl(GV, TM) &&
+          (Kind.isDataRel() || Kind.isBSS() || Kind.isCommon()));
+}
 
+/// Return true if this global address should be placed into small data/bss
+/// section. This method does all the work, except for checking the section
+/// kind.
+bool MipsTargetObjectFile::
+IsGlobalInSmallSectionImpl(const GlobalValue *GV,
+                           const TargetMachine &TM) const {
   const MipsSubtarget &Subtarget = TM.getSubtarget<MipsSubtarget>();
 
   // Return if small section is not available.
   if (!Subtarget.useSmallSection())
     return false;
 
   // Only global variables, not functions.
   const GlobalVariable *GVA = dyn_cast<GlobalVariable>(GV);
   if (!GVA)
     return false;
 
-  // We can only do this for datarel or BSS objects for now.
-  if (!Kind.isBSS() && !Kind.isDataRel())
+  // Enforce -mlocal-sdata.
+  if (!LocalSData && GV->hasLocalLinkage())
     return false;
 
-  // If this is a internal constant string, there is a special
-  // section for it, but not in small data/bss.
-  if (Kind.isMergeable1ByteCString())
+  // Enforce -mextern-sdata.
+  if (!ExternSData && ((GV->hasExternalLinkage() && GV->isDeclaration()) ||
+                       GV->hasCommonLinkage()))
     return false;
 
   Type *Ty = GV->getType()->getElementType();
   return IsInSmallSection(
       TM.getSubtargetImpl()->getDataLayout()->getTypeAllocSize(Ty));
 }
 
-
-
 const MCSection *MipsTargetObjectFile::
 SelectSectionForGlobal(const GlobalValue *GV, SectionKind Kind,
                        Mangler &Mang, const TargetMachine &TM) const {
   // TODO: Could also support "weak" symbols as well with ".gnu.linkonce.s.*"
   // sections?
 
   // Handle Small Section classification here.
   if (Kind.isBSS() && IsGlobalInSmallSection(GV, TM, Kind))
     return SmallBSSSection;
-  if (Kind.isDataNoRel() && IsGlobalInSmallSection(GV, TM, Kind))
+  if (Kind.isDataRel() && IsGlobalInSmallSection(GV, TM, Kind))
     return SmallDataSection;
 
   // Otherwise, we work the same as ELF.
   return TargetLoweringObjectFileELF::SelectSectionForGlobal(GV, Kind, Mang,TM);
 }
+
+/// Return true if this constant should be placed into small data section.
+bool MipsTargetObjectFile::
+IsConstantInSmallSection(const Constant *CN, const TargetMachine &TM) const {
+  return (TM.getSubtarget<MipsSubtarget>().useSmallSection() &&
+          LocalSData &&
+          IsInSmallSection(TM.getSubtargetImpl()->getDataLayout()
+                           ->getTypeAllocSize(CN->getType())));
+}
+
+const MCSection *MipsTargetObjectFile::
+getSectionForConstant(SectionKind Kind, const Constant *C) const {
+  if (IsConstantInSmallSection(C, *TM))
+    return SmallDataSection;
+
+  // Otherwise, we work the same as ELF.
+  return TargetLoweringObjectFileELF::getSectionForConstant(Kind, C);
+}

llvm/trunk/test/CodeGen/Mips/small-section-reserve-gp.ll

-; RUN: llc -mtriple=mipsel-sde-elf -march=mipsel -relocation-model=static < %s \
+; RUN: llc -mtriple=mipsel-sde-elf -march=mipsel -relocation-model=static -mattr=+noabicalls -mgpopt < %s \
 ; RUN: | FileCheck %s
 
 @i = internal unnamed_addr global i32 0, align 4
 
 define i32 @geti() nounwind readonly {
 entry:
 ; CHECK: lw ${{[0-9]+}}, %gp_rel(i)($gp)
   %0 = load i32* @i, align 4
   ret i32 %0
 }