llvm dev - Mar 2011 - [LLVMdev] Calls to functions with signext/zeroext return values

In SelectionDAGBuilder::visitRet(), there is this bit of code:

        // FIXME: C calling convention requires the return type to be promoted
        // to at least 32-bit. But this is not necessary for non-C calling
        // conventions. The frontend should mark functions whose return values
        // require promoting with signext or zeroext attributes.
        if (ExtendKind != ISD::ANY_EXTEND && VT.isInteger()) {
          EVT MinVT = TLI.getRegisterType(*DAG.getContext(), MVT::i32);
          if (VT.bitsLT(MinVT))
            VT = MinVT;
        }

There have been a few discussions about this snippet on llvmdev in the
past[1][2][3], and there seems to be a general consensus that the
responsibility for promoting to the 'int' type should be transfered to
the front end and the signext/zeroext attributes eliminated. But
that's not what I'm interested in discussing here.

What I'd like to ask about is calls to functions that have a
signext/zeroext attribute on their return value. As far as I can tell,
there isn't any corresponding promotion of the return value to i32 in
visitCall(). Should there be?

I ran into problems in a DSP back end that I'm working on where the
return conventions for i16 and i32 are slightly different (they are
both returned in the same accumulator register, but at different
offsets within the accumulator). The callee promoted the return value
to i32, but the caller was expecting it to be returned as an i16.

So I made some changes to SelectionDAGBuilder (see attached patch) to
truncate return values back to their declared sizes and that seemed to
fix the problems for my DSP backend. But these changes broke some
regression tests in other back ends. Specifically,

    LLVM :: CodeGen/MSP430/2009-11-05-8BitLibcalls.ll
    LLVM :: CodeGen/MSP430/indirectbr.ll
    LLVM :: CodeGen/X86/h-registers-3.ll

The failures in the MSP430 tests are particularly troubling because
they are assertion failures in LegalizeDAG because i32 is not a legal
type.

The X86 failure seems less serious. Based on my limited knowledge of
X86 assembly, it looks like the back end is generating code that works
but that is different from what the test expects.

So my questions:

1. Should visitCall() promote the return value to i32 as is done in visitRet()?
2. If so, any suggestions on how to fix my patch or the MSP430 back
end so it won't crash the MSP430 tests?
3. If not, what options do I have for handling the convention mismatch
in my back end? The only one that I can see is ensuring that i32 and
i16 return values use compatible conventions.

-Ken

[1] http://lists.cs.uiuc.edu/pipermail/llvmdev/2008-February/012840.html
[2] http://lists.cs.uiuc.edu/pipermail/llvmdev/2008-May/014449.html
[3] http://lists.cs.uiuc.edu/pipermail/llvmdev/2009-February/020078.html
-------------- next part --------------
A non-text attachment was scrubbed...
Name: call-extended-return.patch
Type: text/x-patch
Size: 2682 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110316/ef97fbfc/attachment.bin>

On Mar 16, 2011, at 5:58 AM, Ken Dyck wrote:
> In SelectionDAGBuilder::visitRet(), there is this bit of code:
> 
>        // FIXME: C calling convention requires the return type to be
promoted
>        // to at least 32-bit. But this is not necessary for non-C calling
>        // conventions. The frontend should mark functions whose return
values
>        // require promoting with signext or zeroext attributes.
>        if (ExtendKind != ISD::ANY_EXTEND && VT.isInteger()) {
>          EVT MinVT = TLI.getRegisterType(*DAG.getContext(), MVT::i32);
>          if (VT.bitsLT(MinVT))
>            VT = MinVT;
>        }
> 
> There have been a few discussions about this snippet on llvmdev in the
> past[1][2][3], and there seems to be a general consensus that the
> responsibility for promoting to the 'int' type should be transfered
to
> the front end and the signext/zeroext attributes eliminated. But
> that's not what I'm interested in discussing here.
> 
> What I'd like to ask about is calls to functions that have a
> signext/zeroext attribute on their return value. As far as I can tell,
> there isn't any corresponding promotion of the return value to i32 in
> visitCall(). Should there be?
Promoting the return value is unsafe for bool returns on x86-64, which in the
latest revision of the ABI only guarantees that the top 7 bits of the 8-bit
register are 0.

Cameron

On Mar 16, 2011, at 9:31 AM, Cameron Zwarich wrote:
> Promoting the return value is unsafe for bool returns on x86-64, which in
the latest revision of the ABI only guarantees that the top 7 bits of the 8-bit
register are 0.
My comment is a bit off, because the question of what type to make the return
value is somewhat orthogonal to the question of which zext assert we should add.

Cameron

On Wed, Mar 16, 2011 at 8:58 AM, Ken Dyck <kd at kendyck.com>
wrote:
> I ran into problems in a DSP back end that I'm working on where the
> return conventions for i16 and i32 are slightly different (they are
> both returned in the same accumulator register, but at different
> offsets within the accumulator). The callee promoted the return value
> to i32, but the caller was expecting it to be returned as an i16.
>
> So I made some changes to SelectionDAGBuilder (see attached patch) to
> truncate return values back to their declared sizes and that seemed to
> fix the problems for my DSP backend.
That patch has been rendered obsolete by some recent changes by
Cameron. Attached is an updated one.
> So my questions:
>
> ...
> 2. If so, any suggestions on how to fix my patch or the MSP430 back
> end so it won't crash the MSP430 tests?
With Cameron's addition of a getTypeForExtendedInteger() hook in
TargetLowering, I've been able to work around the failures in the
MSP430 back end by overriding the extension size to i16 (see the
attached patch). This seems like it would be the appropriate size for
the architecture -- since the current default of i32 isn't a legal
type -- but I really have no idea what affects it has on the runtime
library or whether it conforms to an official ABI (does one exist?).
Anton, do you have any comments?

-Ken
-------------- next part --------------
A non-text attachment was scrubbed...
Name: call-extended-return.r2.patch
Type: text/x-patch
Size: 2656 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110317/8aab78c2/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: msp430-i16-extend.patch
Type: text/x-patch
Size: 1238 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110317/8aab78c2/attachment-0001.bin>