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

[Daniel Sanders]

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

================
Comment at: lib/Target/Mips/MipsSubtarget.cpp:183-186
@@ -173,2 +182,6 @@
 
-  // Set UseSmallSection.
+  // -mabicalls defaults to false for targets other than Linux and NaCl.
+  if (!(IsLinux || isTargetNaCl())
+      && ((getFeatureBits() & Mips::FeatureNoABICalls) == 0))
+    NoABICalls = true;
+
----------------
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.

================
Comment at: lib/Target/Mips/MipsSubtarget.cpp:194-209
@@ -176,3 +193,18 @@
   //       bare-metal.
-  UseSmallSection = !IsLinux && (TM->getRelocationModel() == Reloc::Static);
+  if (NoABICalls) {
+    if (!GPOpt)
+      SmallSectionThreshold = 0;
+    else if (SSThreshold.getNumOccurrences() > 0)
+      SmallSectionThreshold = SSThreshold;
+    else {
+      // If -mips-ssection-threshold is not specified, it defaults to 8 for
+      // targets other than Linux and NaCl.
+      SmallSectionThreshold = !(IsLinux || isTargetNaCl()) ? 8 : 0;
+    }
+  } else {
+    if (GPOpt)
+      errs() << "warning: cannot use small-data accesses for '-mabicalls'"
+             << "\n";
+    SmallSectionThreshold = 0;
+  }
 }
----------------
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?

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:53
@@ +52,3 @@
+bool MipsTargetObjectFile::IsInSmallSection(uint64_t Size) const {
+  return (Size > 0
+          && Size <= TM->getSubtarget<MipsSubtarget>()
----------------
Could you explain why zero-sized objects are excluded?

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:61
@@ -49,2 +60,3 @@
 bool MipsTargetObjectFile::IsGlobalInSmallSection(const GlobalValue *GV,
                                                 const TargetMachine &TM) const {
+  // We first check the case where global is a declaration, because finding
----------------
Can you fix the indentation while you're here?

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:65
@@ -51,1 +64,3 @@
+  // definitions.
   if (GV->isDeclaration() || GV->hasAvailableExternallyLinkage())
+    return IsGlobalInSmallSectionImpl(GV, TM);
----------------
Are static globals allowed in the small section?

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:76-77
@@ -61,1 +75,4 @@
                        SectionKind Kind) const {
+  return (IsGlobalInSmallSectionImpl(GV, TM)
+          && (Kind.isDataRel() || Kind.isBSS() || Kind.isCommon()));
+}
----------------
Style nit: I believe clang-format would put the && on the previous line.

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:84
@@ -63,1 +83,3 @@
+bool MipsTargetObjectFile::IsGlobalInSmallSectionImpl(const GlobalValue *GV,
+                                                const TargetMachine &TM) const {
   const MipsSubtarget &Subtarget = TM.getSubtarget<MipsSubtarget>();
----------------
Style nit: Indentation

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:102
@@ -81,1 +101,3 @@
+  if (!ExternSData && ((GV->hasExternalLinkage() && GV->isDeclaration())
+                       || GV->hasCommonLinkage()))
     return false;
----------------
Style nit: || should be on previous line

================
Comment at: lib/Target/Mips/MipsTargetObjectFile.cpp:126-142
@@ +125,18 @@
+
+/// 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);
+}
----------------
Just a question: Are constants better off in .rodata where they are protected from accidental writes or .sdata?

http://reviews.llvm.org/D4903