Implement the bulk of returning values in Mips fast-isel
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.
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.
Why is this needed? Variable argument lists don't have any effect on the handling of return values.
This ought to be a MipsCCState object
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?
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.
Nit: Space before '?'
Nit: For consistency with other tests we should probably include the colon following the label name.
Likewise for all the lui lines below
I think I already know the answer to this question but why not use lh instead of lhu+seh?
I think I already know the answer to this question but why not use lb instead of lbu+seb?
Nit: blank lines at EOF
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;
%0 = load i16* @s, align 2
For this test I'm just using rets
; RUN: llc -march=mipsel -relocation-model=pic -O0 -mips-fast-isel -fast-isel-abort -mcpu=mips32r2 \
@i = global i32 75, align 4
; Function Attrs: nounwind
%0 = load i16* @s, align 2 ret i16 %0
; CHECK: lui $[[REG_GPa:[0-9]+]], %hi(_gp_disp)
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
Agreed. I'll fix this in a separate patch.
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.
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
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.
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.
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.
Thanks for confirming what I was thinking.
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.
Already answered above.