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

[reed kotler]

I think that the 8/16 return values are dealt with in a later patch.

I will take a look though. This patch is old and I'm 10 patches ahead 
now so i have to remember
the issues.

Reed


On 01/13/2015 12:43 PM, Daniel Sanders wrote:
> 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/
>
>