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

[Daniel Sanders]

(Please include the context as described at http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface).

I'd like to move MipsABIInfo.{cpp,h} in a separate patch if that's ok with you. This will make it easier to see what the changes are made by this patch.

> so far I've counted five classes holding MipsABIInfo objects

I've looked into this properly and I believe we can reduce this a bit but not as much as I'd hoped. At the moment this patch initializes a MipsABIInfo object in:
* MipsAsmParser::MipsAsmParser()
* MipsDisassemblerBase::MipsDisassemblerBase()
* MipsAsmBackend::createObjectWriter()
* MipsRegInfoRecord::MipsRegInfoRecord()
* MipsTargetELFStreamer::MipsTargetELFStreamer()
* MipsSubtarget::MipsSubtarget()

MipsAsmParser doesn't really need it's own copy of the MipsABIInfo object. It could access it via the target streamers instead. To do this, we would have to move the initialization from MipsTargetELFStreamer::MipsTargetELFStreamer() into createMCStreamer() and pass it into both MipsTargetELFStreamer and MipsTargetAsmStreamer. Then MipsAsmParser can get the ABI with something like getTargetStreamer().getABI().

MipsRegInfoRecord doesn't really need to know the ABI. It only needs to know whether it should emit the record as part of .MIPS.options or .reginfo. We should therefore pass this information in to the MipsRegInfoRecord constructor (from MipsTargetELFStreamer which in turn got it from createMCStreamer) and remove the MipsABIInfo object.

The single check for N64 inside MipsDisassemblerBase's subclasses is actually wrong. It should be testing for FeatureGP64bit. Once this is fixed, this class won't need to know the ABI.

These refactors (which should be done in separate patches) leave us with these three initializations.
* MipsAsmBackend::createObjectWriter()
* createMCStreamer()
* MipsSubtarget::MipsSubtarget()
I don't think it's possible to eliminate any more than that. With the constructor change as well (see below), the implementation will end up inside the MipsABIInfo constructor so this isn't too bad.

Do you mind doing those refactors before this patch?

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsABIInfo.cpp:56-66
@@ +55,13 @@
+
+MipsABIInfo::MipsABIInfo(bool is64Bit) {
+  ThisABI = is64Bit ? ABI::N64 : ABI::O32;
+}
+MipsABIInfo::MipsABIInfo() {
+
+  ThisABI = StringSwitch<ABI>(TargetABI.getValue())
+    .Cases("32", "o32", ABI::O32)
+    .Case("n32", ABI::N32)
+    .Cases("64", "n64", ABI::N64)
+    .Default(ABI::Unknown);
+}
+
----------------
All the uses of MipsABIInfo use the following pattern:
  MipsABIInfo ABI; // <- Default constructor reads the TargetABI option
  if (!ABI.IsKnown())
    ABI = MipsABIInfo(IsGP64Bit);
sometimes this is split across functions but we never use a MipsABIInfo until we've done the if-statement.

Rather than having two constructors like this, I think it would be best to merge them into one like so:
  MipsABIInfo::MipsABIInfo(bool Is64bit) {
    ThisABI = StringSwitch<ABI>(TargetABI.getValue())
      .Cases("32", "o32", ABI::O32)
      .Case("n32", ABI::N32)
      .Cases("64", "n64", ABI::N64)
      .Default(ABI::Unknown);
    if (!IsKnown())
      ThisABI = Is64Bit ? ABI::N64 : ABI::O32;
  }
This makes it easy to see that it is impossible to use a partially constructed object.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:145-155
@@ -143,1 +144,13 @@
+
+  MipsABIInfo ABIInfo;
+  if (!ABIInfo.IsKnown()) {
+    bool isCPU64Bit = false;
+    if (CPU.startswith("mips64"))
+      isCPU64Bit = true;
+    else isCPU64Bit = StringSwitch<bool>(CPU)
+        .Cases("mips3", "mips4", "mips5", true)
+        .Default(false);
+    ABIInfo = MipsABIInfo(isCPU64Bit);
+  }
+
   return createMipsELFObjectWriter(OS,
----------------
See comment on MipsABIInfo constructor.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsAsmBackend.cpp:157
@@ -143,3 +156,3 @@
   return createMipsELFObjectWriter(OS,
-    MCELFObjectTargetWriter::getOSABI(OSType), IsLittle, Is64Bit);
+    MCELFObjectTargetWriter::getOSABI(OSType), IsLittle, Is64Bit, ABIInfo.IsN64());
 }
----------------
We should be testing for properties of the ABI rather than the ABI itself. E.g. ABIInfo.UsesELF64Objects() rather than ABIInfo.IsN64().

The existing places where we test for the ABI need updating to this style but that's for another patch-series.

================
Comment at: lib/Target/Mips/MCTargetDesc/MipsTargetStreamer.cpp:344-346
@@ -343,2 +343,5 @@
 
+  //Update ABI info
+  if (!ABIInfo.IsKnown())
+    ABIInfo = MipsABIInfo(Features & Mips::FeatureGP64Bit);
   // Update e_header flags
----------------
See comment in MipsABIInfo constructor

================
Comment at: lib/Target/Mips/MipsSubtarget.cpp:130-131
@@ -141,2 +129,4 @@
     report_fatal_error("Code generation for MIPS-V is not implemented", false);
+  if (!MipsABI.IsKnown())
+    MipsABI = MipsABIInfo(isGP64bit());
 
----------------
See comment on MipsABIInfo constructor

================
Comment at: lib/Target/Mips/MipsSubtarget.h:48
@@ -47,3 +47,3 @@
 
-  // Selected ABI
-  MipsABIInfo ABI;
+  // Mips supported ABIs
+  MipsABIInfo MipsABI;
----------------
This comment is wrong, it's the selected ABI. Please change it back.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:49
@@ -50,1 +48,3 @@
+  // Mips supported ABIs
+  MipsABIInfo MipsABI;
 
----------------
I don't see a need to rename this member.

================
Comment at: lib/Target/Mips/MipsTargetStreamer.h:110-112
@@ -105,1 +109,5 @@
+
+	bool isO32() const { return ABIInfo.IsO32(); }
+	bool isN32() const { return ABIInfo.IsN32(); }
+	bool isN64() const { return ABIInfo.IsN64(); }
 
----------------
Indendation should be spaces.

http://reviews.llvm.org/D6476