Add bulk of returning of values to Mips fast-isel

[rkotler]

Summary

Implement the bulk of returning values in Mips fast-isel

[rkotler]

Executable test.

retabidrv.c303 BDownload

retabi.c226 BDownload

[rkotler]

Update returning of values patch to tip of tree and add test case.
This patch is almost identical to the AArch64 implementation. I am continuing to review this patch myself but hope to push it soon after review.

[dsanders]

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.

lib/Target/Mips/MipsFastISel.cpp

994–996

Why is this needed? Variable argument lists don't have any effect on the handling of return values.

1003

This ought to be a MipsCCState object

1014–1017

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?

1184–1196

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.

1195

Nit: Space before '?'

test/CodeGen/Mips/Fast-ISel/retabi.ll

14

Nit: For consistency with other tests we should probably include the colon following the label name.

17

Nit: indentation

Likewise for all the lui lines below

35–36

I think I already know the answer to this question but why not use lh instead of lhu+seh?

50–51

I think I already know the answer to this question but why not use lb instead of lbu+seb?

82–83

Nit: blank lines at EOF

[rkotler]

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

[rkotler]

lib/Target/Mips/MipsFastISel.cpp

1014–1017

I'm not sure exactly what you are asking here.If you debug this you will see that these tests pass.

1005 CCAssignFn *RetCC = RetCC_Mips;
(gdb)
1006 CCInfo.AnalyzeReturn(Outs, RetCC);
(gdb)
1009 if (ValLocs.size() != 1)
(gdb)
1012 CCValAssign &VA = ValLocs[0];
(gdb)
1013 const Value *RV = Ret->getOperand(0);
(gdb)
1016 if ((VA.getLocInfo() != CCValAssign::Full) &&
(gdb) call RV->dump()

%0 = load i16* @s, align 2

(gdb) next
1021 if (!VA.isRegLoc())
(gdb) print VA.getLocInfo()
$1 = llvm::CCValAssign::Full
(gdb)

For this test I'm just using rets

; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
; RUN: < %s | FileCheck %s

@i = global i32 75, align 4
@s = global i16 -345, align 2
@c = global i8 118, align 1
@f = global float 0x40BE623360000000, align 4
@d = global double 1.298330e+03, align 8

; Function Attrs: nounwind
define signext i16 @rets() {
entry:
; CHECK-LABEL: rets

%0 = load i16* @s, align 2
ret i16 %0

; CHECK: lui $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
; CHECK: addiu $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
; CHECK: addu $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
; 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
}

Fast-isel (not just the mips port) uses a kind of fake legalization to deal with sizes that are not supported directly by the machine like chars and shorts. It's a little
suspect to me but it seems to work and this patch plus the next 12 after this, pass all of test-suite. Even small mistakes in any type of return types not being
sign extended properly will cause test-suite failures so I think everything is fine here.

1184–1196

Agreed. I'll fix this in a separate patch.

1184–1196

These two functions have different numbers of parameters.

In once case you pass the destination register and it returns boolean if it was successful.

In the other case it allocates a destination register and returns it, returning 0 if it was unable to. This is a common convention in fast-isel.

Depending on how this function is used, it makes sense to let it allocation the destination or to allocate it yourself.

test/CodeGen/Mips/Fast-ISel/retabi.ll

35–36

This is happening from different steps combining. This is not an optimizing pass, it's fast-isel.

As long as 16 bit quantity is just loaded into a register, it does not need to be
sign extended. later if it is converted to a 32 bit quantify, then the half word needs
to be sign extended.

[dsanders]

lib/Target/Mips/MipsFastISel.cpp

1014–1017

I believe I've found the root of my confusion. The promotion from i16 to i32 is being handled by GetReturnInfo() and it is this function that sets the SExt flag in CCValAssign. The strange bit is our SelectionDAG implementation never calls this function nor the function that would normally call it for (TargetLowering::LowerCallTo()). This is going to need looking into at some point.

I see how this code works now. It's promoting types in a different from the way our SelectionDAG implementation which worries me a little but it does look like it's doing the right thing.

Even small mistakes in any type of return types not being sign extended properly will cause test-suite failures so I think everything is fine here.

Not necessarily. If both caller and callee agree to do the wrong thing then the test-suite will pass despite the calling convention being wrong. Big-endian N32 and N64 had several examples of this, and there was one in O32 too. One concrete example, is if the callee sign-extends when it isn't supposed to and the caller doesn't sign-extend when it is supposed to. In this situation, calling clang-compiled code from clang-compiled code will work but calling gcc-compiled code will not.

1184–1196

You've already said you'll fix this in a later patch but you followed up with another comment that appeared to be implying that it's not a problem. Apologies if I've read your second comment incorrectly.

Being given a register number vs allocating one internally isn't the issue. The problem is that the return types can't be used in the same way.

Consider the following code:

unsigned ResultReg = emitIntExt(SrcVT, SrcReg, DestVT, isZExt);
if (ResultReg)
  updateValueMap(I, ResultReg);

and this code:

unsigned ResultReg = emitIntExt(SrcVT, SrcReg, DestVT, ResultReg, isZExt);
if (ResultReg)
  updateValueMap(I, ResultReg);

Both look correct and compile but the second one is wrong since ResultReg is actually 0 or 1. If we want the function to be overloaded then we need to return semantically compatible types so that we don't leave this kind of trap for other programmers.

test/CodeGen/Mips/Fast-ISel/retabi.ll

35–36

Thanks for confirming what I was thinking.

[rkotler]

Fixed issues from last review.

[dsanders]

This patch LGTM. I do still have some concerns about the way our SelectionDAG and FastISel are handling sign/zero extended types in the calling convention differently but that shouldn't block this patch.

Also, please follow up on the overloaded function issue as soon as you can.

lib/Target/Mips/MipsFastISel.cpp

994–996

Done

1003

Done

1195

Done

test/CodeGen/Mips/Fast-ISel/retabi.ll

14

Done

17

Done

50–51

Already answered above.

82–83

Done

lib/Target/Mips/MipsFastISel.cpp

 //===-- MipsastISel.cpp - Mips FastISel implementation
 //---------------------===//
 
-#include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "MipsCCState.h"
 #include "MipsISelLowering.h"
 #include "MipsMachineFunction.h"
 #include "MipsRegisterInfo.h"
 #include "MipsSubtarget.h"
 #include "MipsTargetMachine.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/CodeGen/FastISel.h"
+#include "llvm/CodeGen/FunctionLoweringInfo.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/Target/TargetInstrInfo.h"
 
 using namespace llvm;
 
 namespace {
 
bool MipsFastISel::fastLowerCall(CallLoweringInfo &CLI) {
   MIB.addRegMask(TRI.getCallPreservedMask(CC));
 
   CLI.Call = MIB;
   // Finish off the call including any return values.
   return finishCall(CLI, RetVT, NumBytes);
 }
 
 bool MipsFastISel::selectRet(const Instruction *I) {
+  const Function &F = *I->getParent()->getParent();
   const ReturnInst *Ret = cast<ReturnInst>(I);
 
   if (!FuncInfo.CanLowerReturn)
     return false;
+
+  // Build a list of return value registers.
+  SmallVector<unsigned, 4> RetRegs;
+
   if (Ret->getNumOperands() > 0) {
+    CallingConv::ID CC = F.getCallingConv();
+    SmallVector<ISD::OutputArg, 4> Outs;
+    GetReturnInfo(F.getReturnType(), F.getAttributes(), Outs, TLI);
+    // Analyze operands of the call, assigning locations to each operand.
+    SmallVector<CCValAssign, 16> ValLocs;
+    MipsCCState CCInfo(CC, F.isVarArg(), *FuncInfo.MF, ValLocs,
+                       I->getContext());
+    CCAssignFn *RetCC = RetCC_Mips;
+    CCInfo.AnalyzeReturn(Outs, RetCC);
+
+    // Only handle a single return value for now.
+    if (ValLocs.size() != 1)
+      return false;
+
+    CCValAssign &VA = ValLocs[0];
+    const Value *RV = Ret->getOperand(0);
+
+    // Don't bother handling odd stuff for now.
+    if ((VA.getLocInfo() != CCValAssign::Full) &&
+        (VA.getLocInfo() != CCValAssign::BCvt))
+      return false;
+
+    // Only handle register returns for now.
+    if (!VA.isRegLoc())
+      return false;
+
+    unsigned Reg = getRegForValue(RV);
+    if (Reg == 0)
+      return false;
+
+    unsigned SrcReg = Reg + VA.getValNo();
+    unsigned DestReg = VA.getLocReg();
+    // Avoid a cross-class copy. This is very unlikely.
+    if (!MRI.getRegClass(SrcReg)->contains(DestReg))
+      return false;
+
+    EVT RVEVT = TLI.getValueType(RV->getType());
+    if (!RVEVT.isSimple())
       return false;
+
+    if (RVEVT.isVector())
+      return false;
+
+    MVT RVVT = RVEVT.getSimpleVT();
+    if (RVVT == MVT::f128)
+      return false;
+
+    MVT DestVT = VA.getValVT();
+    // Special handling for extended integers.
+    if (RVVT != DestVT) {
+      if (RVVT != MVT::i1 && RVVT != MVT::i8 && RVVT != MVT::i16)
+        return false;
+
+      if (!Outs[0].Flags.isZExt() && !Outs[0].Flags.isSExt())
+        return false;
+
+      bool IsZExt = Outs[0].Flags.isZExt();
+      SrcReg = emitIntExt(RVVT, SrcReg, DestVT, IsZExt);
+      if (SrcReg == 0)
+        return false;
+    }
+
+    // Make the copy.
+    BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc,
+            TII.get(TargetOpcode::COPY), DestReg).addReg(SrcReg);
+
+    // Add register to return instruction.
+    RetRegs.push_back(VA.getLocReg());
   }
-  emitInst(Mips::RetRA);
+  MachineInstrBuilder MIB = emitInst(Mips::RetRA);
+  for (unsigned i = 0, e = RetRegs.size(); i != e; ++i)
+    MIB.addReg(RetRegs[i], RegState::Implicit);
   return true;
 }
 
 bool MipsFastISel::selectTrunc(const Instruction *I) {
   // The high bits for a type smaller than the register size are assumed to be
   // undefined.
   Value *Op = I->getOperand(0);
 
case MVT::i8:
     break;
   case MVT::i16:
     emitInst(Mips::ANDi, DestReg).addReg(SrcReg).addImm(0xffff);
     break;
   }
   return true;
 }
 
 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;
 }
 
 bool MipsFastISel::fastSelectInstruction(const Instruction *I) {
   if (!TargetSupported)
     return false;
   switch (I->getOpcode()) {
   default:
     break;
   case Instruction::Load:

test/CodeGen/Mips/Fast-ISel/retabi.ll

+; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
+; RUN:     < %s | FileCheck %s
+
+@i = global i32 75, align 4
+@s = global i16 -345, align 2
+@c = global i8 118, align 1
+@f = global float 0x40BE623360000000, align 4
+@d = global double 1.298330e+03, align 8
+
+; Function Attrs: nounwind
+define i32 @reti() {
+entry:
+; CHECK-LABEL: reti:
+  %0 = load i32* @i, align 4
+  ret i32 %0
+; CHECK:        lui     $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:        addiu   $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK:        addu    $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; CHECK:        lw      $[[REG_I_ADDR:[0-9]+]], %got(i)($[[REG_GP]])
+; CHECK:        lw      $2, 0($[[REG_I_ADDR]])
+; CHECK:        jr      $ra
+}
+
+; Function Attrs: nounwind
+define signext i16 @rets() {
+entry:
+; CHECK-LABEL: rets:
+  %0 = load i16* @s, align 2
+  ret i16 %0
+; CHECK:        lui     $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:        addiu   $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK:        addu    $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; 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
+}
+
+; Function Attrs: nounwind
+define signext i8 @retc() {
+entry:
+; CHECK-LABEL: retc:
+  %0 = load i8* @c, align 1
+  ret i8 %0
+; CHECK:        lui     $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:        addiu   $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK:        addu    $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; 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
+}
+
+; Function Attrs: nounwind
+define float @retf() {
+entry:
+; CHECK-LABEL: retf:
+  %0 = load float* @f, align 4
+  ret float %0
+; CHECK:        lui     $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:        addiu   $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK:        addu    $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; CHECK:        lw      $[[REG_F_ADDR:[0-9]+]], %got(f)($[[REG_GP]])
+; CHECK:        lwc1    $f0, 0($[[REG_F_ADDR]])
+; CHECK:        jr      $ra
+}
+
+; Function Attrs: nounwind
+define double @retd() {
+entry:
+; CHECK-LABEL: retd:
+  %0 = load double* @d, align 8
+  ret double %0
+; CHECK:        lui     $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
+; CHECK:        addiu   $[[REG_GPb:[0-9]+]], $[[REG_GPa]], %lo(_gp_disp)
+; CHECK:        addu    $[[REG_GP:[0-9]+]], $[[REG_GPb]], $25
+; CHECK:        lw      $[[REG_D_ADDR:[0-9]+]], %got(d)($[[REG_GP]])
+; CHECK:        ldc1    $f0, 0($[[REG_D_ADDR]])
+; CHECK:        jr      $ra
+}