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

[reed kotler]

Adding comments responding to Daniels comments.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:770
@@ +769,3 @@
+
+  emitInst(Mips::ADJCALLSTACKDOWN).addImm(16);
+  // Process the args.
----------------
dsanders wrote:
> The 16-bytes reserved space is allocated by the prologue of the callee not the caller.
> 
> However, I discovered something strange in the SelectionDAG implementation this morning. MipsTargetLowering::LowerCall() calls CCInfo.AllocateStack() to allocate the reserved argument area even though LowerCall is lowering from the callers point of view. Removing the call breaks the frame size calculation so it's clearly important but I haven't figured out why. I would have expected the one in MipsTargetLowering::LowerFormalArguments() to be the important one.
For O32, I think it's the caller.
>From the ABI Doc:

https://dmz-portal.mips.com/mw/images/f/fe/MD00305-2B-ABIDESC-SPC-01.03.pdf

7. The caller will always build an argument data
structure, even though it may remain unused in whole
or part. Moreover, the data structure is always a
minimum of 16 bytes (four register-sized slots) in
size.



================
Comment at: lib/Target/Mips/MipsFastISel.cpp:770
@@ +769,3 @@
+
+  emitInst(Mips::ADJCALLSTACKDOWN).addImm(16);
+  // Process the args.
----------------
rkotler wrote:
> dsanders wrote:
> > The 16-bytes reserved space is allocated by the prologue of the callee not the caller.
> > 
> > However, I discovered something strange in the SelectionDAG implementation this morning. MipsTargetLowering::LowerCall() calls CCInfo.AllocateStack() to allocate the reserved argument area even though LowerCall is lowering from the callers point of view. Removing the call breaks the frame size calculation so it's clearly important but I haven't figured out why. I would have expected the one in MipsTargetLowering::LowerFormalArguments() to be the important one.
> For O32, I think it's the caller.
> From the ABI Doc:
> 
> https://dmz-portal.mips.com/mw/images/f/fe/MD00305-2B-ABIDESC-SPC-01.03.pdf
> 
> 7. The caller will always build an argument data
> structure, even though it may remain unused in whole
> or part. Moreover, the data structure is always a
> minimum of 16 bytes (four register-sized slots) in
> size.
> 
> 
BTW, with this patch, I pass all of test-suite at O0/O2 for mips32r1 and mips32r2.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:828-829
@@ +827,4 @@
+    }
+    case CCValAssign::AExt:
+    // Intentional fall-through.
+    case CCValAssign::ZExt: {
----------------
dsanders wrote:
> It makes no difference for 32-bit but AExt is normally treated the same way as SExt by our backend.
What do you want here?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:843-849
@@ +842,9 @@
+    // Now copy/store arg to correct locations.
+    if (VA.isRegLoc() && !VA.needsCustom()) {
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+              TII.get(TargetOpcode::COPY), VA.getLocReg()).addReg(ArgReg);
+      CLI.OutRegs.push_back(VA.getLocReg());
+    } else if (VA.needsCustom()) {
+      // FIXME: Handle custom args.
+      return false;
+    } else {
----------------
dsanders wrote:
> We don't use custom arguments. We ought to delete the references to VA.needsCustom()
It seems we should then make this llvm_unreachable then. If someone later changes the compiler and we have custom args, we want to fail here because that is our assumption.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:878-879
@@ +877,4 @@
+
+  if (NumBytes < 16)
+    NumBytes = 16;
+  return true;
----------------
dsanders wrote:
> Didn't we do this on line 767 too?
Yes. Probably a merge error when I went to the AArch64 model of fast-isel.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:971
@@ +970,3 @@
+  MachineInstrBuilder MIB =
+      BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(Mips::JALR),
+              Mips::RA).addReg(Mips::T9);
----------------
dsanders wrote:
> This isn't correct for MIPS32r6 and I've realised that we don't guard against MIPS32r6. hasMips32() is true for MIPS32, MIPS32r2, MIPS32r6, MIPS64, MIPS64r2, and MIPS64r6. Just correct the main guard for now.
Ok.

http://reviews.llvm.org/D5714