[PATCH] [Mips][ABI]The ELF container needs to depend on the ABI rather than the target triple.

[Daniel Sanders]

Thanks for doing those refactors. There's quite a few comments below. Many are nits but there's a couple important ones.

As you mentioned when we spoke off-list, we still need test cases for this. An assembler test that verifies that mips/mips64 triples have no effect on the ELF32/ELF64-ness of the ELF output should do.


================
Comment at: include/llvm/MC/MCParser/MCAsmParserExtension.h:68-71
@@ -67,2 +67,6 @@
   MCStreamer &getStreamer() { return getParser().getStreamer(); }
+  const MCStreamer &getStreamer() const {
+    return const_cast<MCAsmParserExtension*>(this)->getStreamer();
+  }
+
   bool Warning(SMLoc L, const Twine &Msg) {
----------------
This new function isn't needed (deleting it doesn't break the build) and just calls the existing getStreamer() above after doing a dangerous cast.

================
Comment at: include/llvm/MC/MCStreamer.h:233-236
@@ -232,2 +232,6 @@
 
+  const MCTargetStreamer *getTargetStreamer() const {
+    return const_cast<MCStreamer *>(this)->getTargetStreamer();
+  }
+
   unsigned getNumFrameInfos() { return DwarfFrameInfos.size(); }
----------------
Deleting this doesn't break the build and that cast is dangerous.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:97-100
@@ -96,2 +96,6 @@
 
+  const MipsTargetStreamer &getTargetStreamer() const {
+    return const_cast<MipsAsmParser *>(this)->getTargetStreamer();
+  }
+
   MCSubtargetInfo &STI;
----------------
This does seem to be needed but the cast is dangerous, we need to find another way.

It looks like we're going to have to cache the MipsABIInfo object from the target streamer during the MipsAsmParser constructor. Fortunately, it shouldn't change during execution.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:334-335
@@ -329,9 +333,4 @@
 
-    getTargetStreamer().updateABIInfo(*this);
-
-    // Assert exactly one ABI was chosen.
-    assert((((STI.getFeatureBits() & Mips::FeatureO32) != 0) +
-            ((STI.getFeatureBits() & Mips::FeatureEABI) != 0) +
-            ((STI.getFeatureBits() & Mips::FeatureN32) != 0) +
-            ((STI.getFeatureBits() & Mips::FeatureN64) != 0)) == 1);
+    getTargetStreamer().updateABIFlagsSection(
+                        static_cast<const MipsAsmParser&>(*this));
 
----------------
I don't see why this cast is needed.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp:50-55
@@ +49,8 @@
+
+cl::opt<std::string> TargetABI(
+  "target-abi",
+  cl::init(""),
+  cl::desc("ABI used"),
+  cl::Hidden);
+
+MipsABIInfo::MipsABIInfo(bool is64Bit) {
----------------
You'll need to resolve a conflict with the 'target-abi' option Eric Christopher added in r224492. It's normally accessed through the TargetOptions class.

I tried a couple quick fixes but didn't successfully get 'make check' passing.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp:56
@@ +55,3 @@
+
+MipsABIInfo::MipsABIInfo(bool is64Bit) {
+  ThisABI = StringSwitch<ABI>(TargetABI.getValue())
----------------
Nit: Variables normally start with a capital

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsABIInfo.h:30
@@ -27,2 +29,3 @@
   MipsABIInfo(ABI ThisABI) : ThisABI(ThisABI) {}
+  MipsABIInfo(bool is64bit);
 
----------------
Nit: Variables normally start with capitals.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:145-150
@@ +144,8 @@
+
+  bool isCPU64Bit = false;
+  if (CPU.startswith("mips64"))
+    isCPU64Bit = true;
+  else isCPU64Bit = StringSwitch<bool>(CPU)
+      .Cases("mips3", "mips4", "mips5", true)
+      .Default(false);
+  MipsABIInfo ABIInfo = MipsABIInfo(isCPU64Bit);
----------------
This should probably be in MipsABIInfo just to keep it near to MipsABIInfo::selectMipsCPU(). We ought to find a proper home for both at some point.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:409
@@ -398,2 +408,3 @@
 
+
 // MCAsmBackend
----------------
Nit: You've added an unnecessary blank line here

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsELFObjectWriter.cpp:13
@@ -12,2 +12,3 @@
 #include "MCTargetDesc/MipsMCTargetDesc.h"
+#include "MipsABIInfo.h"
 #include "llvm/MC/MCAssembler.h"
----------------
I don't think you need this include.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsMCTargetDesc.cpp:117-125
@@ -128,9 +116,11 @@
       Ctx, OS, isVerboseAsm, useDwarfDirectory, InstPrint, CE, TAB, ShowInst);
-  new MipsTargetAsmStreamer(*S, OS);
+  MipsABIInfo ABI(false);
+  new MipsTargetAsmStreamer(*S, ABI, OS);
   return S;
 }
 
 static MCStreamer *createMipsNullStreamer(MCContext &Ctx) {
   MCStreamer *S = llvm::createNullStreamer(Ctx);
-  new MipsTargetStreamer(*S);
+  MipsABIInfo ABI(false);
+  new MipsTargetStreamer(*S,ABI);
   return S;
----------------
These will both do the right thing (at the moment) since it only matters for ELF output but I'm not happy with the idea that the ABI can be different between ELF and Assembly output, nor that it can be O32 when we are really doing N32/N64.

================
Comment at: lib/Target/Mips/MipsSubtarget.cpp:209
@@ -227,1 +208,2 @@
 }
+
----------------
Nit: You seem to have added a blank line here. Intentional? The context is missing so I can't tell if it's the end of the file or next to another blank line.

http://reviews.llvm.org/D6476

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/