[PATCH] First stage of call lowering for Mips fast-isel

[Daniel Sanders]

Thanks. LGTM with a few nits and a couple minor changes.

I'll unify the two O32 implementations as I work my way through the O32 refactoring.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:97
@@ -94,1 +96,3 @@
+  bool computeCallAddress(const Value *V, Address &Addr);
+
 
----------------
Nit: Too many blank lines

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:187
@@ +186,3 @@
+
+
+using namespace llvm;
----------------
Nit: Too many blank lines

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:188
@@ +187,3 @@
+
+using namespace llvm;
+
----------------
Nit: You already have this at the top of the file.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:190-200
@@ +189,13 @@
+
+static bool CC_MipsO32_FP32(unsigned ValNo, MVT ValVT,
+                            MVT LocVT, CCValAssign::LocInfo LocInfo,
+                            ISD::ArgFlagsTy ArgFlags, CCState &State) {
+  return false;
+}
+
+bool CC_MipsO32_FP64(unsigned ValNo, MVT ValVT,
+                            MVT LocVT, CCValAssign::LocInfo LocInfo,
+                            ISD::ArgFlagsTy ArgFlags, CCState &State) {
+  return false;
+}
+
----------------
I'd prefer these to contain llvm_unreachable so that we can easily demonstrate that Fast ISel doesn't use them.

In case anyone wonders why these functions exist: They are a side-effect of my recent ABI refactoring work. I added CustomCallingConv as a way to migrate the O32 implementation to a tablegen-erated definition without having to do it all at once. This worked nicely when only one file #included MipsGenCallingConv.inc but now MipsFastISel.cpp does it too and has to provide an implementation of the static functions that tablegen doesn't provide. Fortunately, this patch doesn't need to use CC_MipsO32_F{32,64} so we can just use stub functions for the moment. I'll clean this up as I work through the O32 implementation.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:201
@@ +200,3 @@
+}
+
+
----------------
Nit: Too many blank lines.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:749
@@ -695,2 +748,3 @@
 }
 //
+bool MipsFastISel::processCallArgs(CallLoweringInfo &CLI,
----------------
Nit: Empty comment and missing blank line.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:753
@@ +752,3 @@
+                                   unsigned &NumBytes) {
+  // for now we don't handle any arguments
+  CallingConv::ID CC = CLI.CallConv;
----------------
Nit: Capitalization in comment and missing punctuation

There are a few more instances of this in this function.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:841
@@ +840,3 @@
+    } else if (VA.needsCustom()) {
+        llvm_unreachable("Mips does not use custom args.");
+      return false;
----------------
I agree that using llvm_unreachable() is better than what I suggested.

Nit: Indentation

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:846
@@ +845,3 @@
+      // FIXME: This path will currently return false. It was copied
+      // from the AARch64 port and should be essentially fine for Mips too
+      // but the finish work on it will be enabled in a follow on patch.
----------------
Nit: AArch64 not AARch64. 

Also '... fine for Mips too but the finish work on it will be enabled ...' is rather strangely worded.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:855
@@ +854,3 @@
+      // Need to store on the stack.
+      unsigned ArgSize = (ArgVT.getSizeInBits() + 7) / 8;
+
----------------
You missed this comment: We should use RoundUpToAlignment()

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:876-877
@@ +875,4 @@
+
+  if (NumBytes < 16)
+    NumBytes = 16;
+  return true;
----------------
> Yes. Probably a merge error when I went to the AArch64 model of fast-isel.

Given that you've said this is probably a merge error, shouldn't you remove this?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:964-971
@@ +963,10 @@
+  // Issue the call.
+  // emitInst(Mips::ADDu, Mips::GP).addReg(MFI->getGlobalBaseReg()).addReg(
+  //    Mips::ZERO);
+  unsigned DestAddress = materializeGV(Addr.getGlobalValue(), MVT::i32);
+  emitInst(TargetOpcode::COPY, Mips::T9).addReg(DestAddress);
+  MachineInstrBuilder MIB =
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Mips::JALR),
+              Mips::RA).addReg(Mips::T9);
+  //    Mips::RA).addReg(DestAddress)
+
----------------
I missed this the first time around. Could you delete the commented out code?

================
Comment at: lib/Target/Mips/MipsISelLowering.cpp:2364
@@ -2359,1 +2363,3 @@
+
+
 #include "MipsGenCallingConv.inc"
----------------
Nit: Too many blank lines

http://reviews.llvm.org/D5714