[PATCH] Add bulk of returning of values to Mips fast-isel

[Daniel Sanders]

Sorry for being so slow to review this one. I think this patch is heading in the right direction but there's something strange about the i8/i16 cases that I'd like you to explain. Details inline.

It also rejects quite a lot of the cases (including some easy ones like any-extend) but we can expand on it in follow up patches.


================
Comment at: lib/Target/Mips/MipsFastISel.cpp:994-996
@@ +993,5 @@
+
+  if (F.isVarArg())
+      return false;
+
+  // Build a list of return value registers.
----------------
Why is this needed? Variable argument lists don't have any effect on the handling of return values.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1006
@@ +1005,3 @@
+    SmallVector<CCValAssign, 16> ValLocs;
+    CCState CCInfo(CC, F.isVarArg(), *FuncInfo.MF, ValLocs, I->getContext());
+    CCAssignFn *RetCC = RetCC_Mips;
----------------
This ought to be a MipsCCState object

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1017-1020
@@ +1016,6 @@
+
+    // Don't bother handling odd stuff for now.
+    if ((VA.getLocInfo() != CCValAssign::Full) &&
+        (VA.getLocInfo() != CCValAssign::BCvt))
+      return false;
+
----------------
Something seems odd here. This condition should be rejecting the rets() and retc() cases from the test case since they should have CCValAssign::SExt. However, the test case is using -fast-isel-abort.

I see that RetCC_O32 doesn't have a CCPromoteToType like the others do which might explain why it isn't SExt but then the question is how does a function guarantee an i16/i8 result is sign extended (and conversely, how does the caller know it doesn't need to do it itself).

Could you explain how the rets() and retc() cases are passing? Are they using CCValAssign::Full here?

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1186-1198
@@ -1107,14 +1185,15 @@
 
 bool MipsFastISel::emitIntExt(MVT SrcVT, unsigned SrcReg, MVT DestVT,
                               unsigned DestReg, bool IsZExt) {
   if (IsZExt)
     return emitIntZExt(SrcVT, SrcReg, DestVT, DestReg);
   return emitIntSExt(SrcVT, SrcReg, DestVT, DestReg);
 }
 
 unsigned MipsFastISel::emitIntExt(MVT SrcVT, unsigned SrcReg, MVT DestVT,
                                   bool isZExt) {
   unsigned DestReg = createResultReg(&Mips::GPR32RegClass);
-  return emitIntExt(SrcVT, SrcReg, DestVT, DestReg, isZExt);
+  bool Success = emitIntExt(SrcVT, SrcReg, DestVT, DestReg, isZExt);
+  return Success? DestReg: 0;
 }
 
----------------
The inconsistency between the return types of these two overloaded functions is going to really confuse someone when they accidentally get boolean 1 instead of a register number. Going by the change in the patch, I suspect it already has.

It doesn't have to be in this patch but please make them consistent ASAP. My preference is to settle on returning the result register since that's usable as a boolean and a register number.

================
Comment at: lib/Target/Mips/MipsFastISel.cpp:1197
@@ -1119,1 +1196,3 @@
+  bool Success = emitIntExt(SrcVT, SrcReg, DestVT, DestReg, isZExt);
+  return Success? DestReg: 0;
 }
----------------
Nit: Space before '?'

================
Comment at: test/CodeGen/Mips/Fast-ISel/retabi.ll:13
@@ +12,3 @@
+entry:
+; CHECK-LABEL: reti
+  %0 = load i32* @i, align 4
----------------
Nit: For consistency with other tests we should probably include the colon following the label name.

================
Comment at: test/CodeGen/Mips/Fast-ISel/retabi.ll:16
@@ +15,3 @@
+  ret i32 %0
+; CHECK:    	lui	$[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:	addiu	$[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
----------------
Nit: indentation

Likewise for all the lui lines below

================
Comment at: test/CodeGen/Mips/Fast-ISel/retabi.ll:34-35
@@ +33,4 @@
+; CHECK:	lw	$[[REG_S_ADDR:[0-9]+]], %got(s)($[[REG_GP]])
+; CHECK:	lhu	$[[REG_S:[0-9]+]], 0($[[REG_S_ADDR]])
+; CHECK:	seh	$2, $[[REG_S]]
+; CHECK:	jr	$ra
----------------
I think I already know the answer to this question but why not use lh instead of lhu+seh?

================
Comment at: test/CodeGen/Mips/Fast-ISel/retabi.ll:49-50
@@ +48,4 @@
+; CHECK:	lw	$[[REG_C_ADDR:[0-9]+]], %got(c)($[[REG_GP]])
+; CHECK:	lbu	$[[REG_C:[0-9]+]], 0($[[REG_C_ADDR]])
+; CHECK:	seb	$2, $[[REG_C]]
+; CHECK:	jr	$ra
----------------
I think I already know the answer to this question but why not use lb instead of lbu+seb?

================
Comment at: test/CodeGen/Mips/Fast-ISel/retabi.ll:81-82
@@ +80,3 @@
+}
+
+
----------------
Nit: blank lines at EOF

http://reviews.llvm.org/D5920

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