Page MenuHomePhabricator

Add bulk of returning of values to Mips fast-isel
ClosedPublic

Authored by rkotler on Oct 22 2014, 3:28 PM.

Details

Diff Detail

Event Timeline

rkotler updated this revision to Diff 15280.Oct 22 2014, 3:28 PM
rkotler retitled this revision from to Add bulk of returning of values to Mips fast-isel.
rkotler updated this object.
rkotler edited the test plan for this revision. (Show Details)
rkotler added a reviewer: dsanders.
rkotler added a subscriber: rfuhler.
rkotler added a subscriber: Unknown Object (MLST).Oct 22 2014, 3:28 PM

Executable test.

rkotler updated this revision to Diff 16255.Nov 14 2014, 4:21 PM

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.

rkotler updated this revision to Diff 17628.Dec 24 2014, 1:11 PM
rkotler updated this object.
dsanders edited edge metadata.Jan 13 2015, 12:43 PM

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

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 added inline comments.Feb 9 2015, 8:25 PM
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 added inline comments.Feb 10 2015, 3:29 AM
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 updated this revision to Diff 19713.Feb 10 2015, 2:48 PM
rkotler edited edge metadata.

Fixed issues from last review.

dsanders accepted this revision.Feb 12 2015, 8:24 AM
dsanders edited edge metadata.

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

This revision is now accepted and ready to land.Feb 12 2015, 8:24 AM
rkotler updated this object.Feb 12 2015, 1:06 PM
rkotler edited the test plan for this revision. (Show Details)
rkotler edited edge metadata.
rkotler closed this revision.Feb 12 2015, 1:07 PM