Ken Dyck
2011-Mar-16 12:58 UTC
[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>
Cameron Zwarich
2011-Mar-16 16:31 UTC
[LLVMdev] Calls to functions with signext/zeroext return values
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
Cameron Zwarich
2011-Mar-16 16:35 UTC
[LLVMdev] Calls to functions with signext/zeroext return values
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
Ken Dyck
2011-Mar-17 13:42 UTC
[LLVMdev] Calls to functions with signext/zeroext return values
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>
Maybe Matching Threads
- [LLVMdev] Calls to functions with signext/zeroext return values
- [LLVMdev] Bug in SelectionDAGBuild.cpp?
- [LLVMdev] Calls to functions with signext/zeroext return values
- [LLVMdev] Calls to functions with signext/zeroext return values
- i32 vs i32 signext for C int parameters to library functions