[PATCH] X86 ABI fix for return values > 24 bytes.

[Quentin Colombet]

On Feb 4, 2015, at 8:50 PM, Andrew Trick <atrick at apple.com> wrote:

> 
>> On Feb 4, 2015, at 5:44 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>> Hi Andy,
>> 
>> I have one question regarding this comment:
>> 
>> +  // Checking Function.hasStructRetAttr() here is insufficient because the IR
>> +  // may not have an explicit sret argument. If FuncInfo.CanLowerReturn is
>> +  // false, then an sret argument may be implicitly inserted in the SelDAG.
>> 
>> When this is set implicitly, how do we know that the caller will have allocated the right among of space to return the structure?
>> 
>> I am not saying your patch is wrong on the contrary, I just wonder how that works because if we made that mistake in the callee, I believe we may have done something silly in the caller too.
> 
> Great question. 
> 
> On the caller side, TargetLowering::LowerCallTo() is responsible for allocating the right amount of space for the sret value. It determines whether implicit sret is needed by calling TLI->CanLowerReturn()
> 
> On the callee side, FunctionLoweringInfo::set() uses the same helper, TLI->CanLowerReturn(), to determine whether implicit sret is needed.
> 
> SelectionDAGISel::LowerArguments checks the CanLowerReturn status, actually insert the implicit sret arg, and sets the SRetReturnReg.
> 
> All of the above was already working and is well tested.

Thanks for checking.

> 
> However, X86TargetLowering::LowerReturn was not checking for an implicit sret argument, and failing to copy the address into %rax in that case. Rather than adding an redundant flag to FuncInfo. It makes sense to query SRetReturnReg, which is set iff an sret argument is needed, regardless of whether it is implicit.

Agreed.
LGTM.

Thanks,
-Quentin

> 
> -Andy
> 
>> 
>> Thanks,
>> -Quentin
>> 
>>> On Feb 4, 2015, at 10:11 AM, Andrew Trick <atrick at apple.com> wrote:
>>> 
>>> The return value's address must be returned in %rax.
>>> i.e. the callee needs to copy the sret argument (%rdi)
>>> into the return value (%rax).
>>> 
>>> This probably won't manifest as a bug when the caller is LLVM-compiled
>>> code. But it is an ABI guarantee and tools expect it.
>>> 
>>> -Andy
>>> 
>>> <0001-X86-ABI-fix-for-return-values-24-bytes.patch>_______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>