<div dir="ltr">On 6 August 2013 06:09, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi nicholas,<br>
<br>
As in PR16742, we should probably be able to handle tail calls like:<br>
bool g(void);<br>
bool f() { return g(); }<br>
<br>
At the moment, since @f ends up returning "zeroext i1" we immediately bail out of trying to make it a tail call. In fact provided @f and @g return the *same* sized result via an extension, the extension required will itself will be the same.<br>
<br>
This patch implements that logic:<br>
+ If the caller (@f) has no extension attribute, anything goes, as before -- the higher bits are undef.<br>
+ If the caller has zeroext/signext and the callee has different, then things will go wrong.<br>
+ Otherwise, the conventions match so make sure the bit-width is the same in each case.<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1299" target="_blank">http://llvm-reviews.chandlerc.com/D1299</a><br>
<br>
Files:<br>
lib/CodeGen/Analysis.cpp<br>
test/CodeGen/X86/sibcall.ll<br>
test/CodeGen/X86/tail-call-exts.ll<br>
<br>
Index: lib/CodeGen/Analysis.cpp<br>
===================================================================<br>
--- lib/CodeGen/Analysis.cpp<br>
+++ lib/CodeGen/Analysis.cpp<br>
@@ -320,6 +320,7 @@<br>
static bool slotOnlyDiscardsData(const Value *RetVal, const Value *CallVal,<br>
SmallVectorImpl<unsigned> &RetIndices,<br>
SmallVectorImpl<unsigned> &CallIndices,<br>
+ bool AllowDifferingSizes,<br>
const TargetLoweringBase &TLI) {<br>
<br>
// Trace the sub-value needed by the return value as far back up the graph as<br>
@@ -350,7 +351,8 @@<br>
// all the bits that are needed by the "ret" have been provided by the "tail<br>
// call". FIXME: with sufficiently cunning bit-tracking, we could look through<br>
// extensions too.<br>
- if (BitsProvided < BitsRequired)<br>
+ if (BitsProvided < BitsRequired ||<br>
+ (!AllowDifferingSizes && BitsProvided != BitsRequired))<br>
return false;<br>
<br>
return true;<br>
@@ -517,20 +519,25 @@<br>
// return type is.<br>
if (isa<UndefValue>(Ret->getOperand(0))) return true;<br>
<br>
- // Conservatively require the attributes of the call to match those of<br>
- // the return. Ignore noalias because it doesn't affect the call sequence.<br>
- const Function *F = ExitBB->getParent();<br>
- AttributeSet CallerAttrs = F->getAttributes();<br>
- if (AttrBuilder(CallerAttrs, AttributeSet::ReturnIndex).<br>
- removeAttribute(Attribute::NoAlias) !=<br>
- AttrBuilder(CallerAttrs, AttributeSet::ReturnIndex).<br>
- removeAttribute(Attribute::NoAlias))<br>
- return false;<br>
+ // If the ABI expects the caller to do some extension of the result before<br>
+ // return, then in general we shouldn't allow a truncate between the call-site<br>
+ // and the ret.<br>
+ AttributeSet CallerAttrs = ExitBB->getParent()->getAttributes();<br>
+ AttributeSet CalleeAttrs = cast<CallInst>(I)->getAttributes();<br></blockquote><div><br></div><div>What if it's an InvokeInst? Or is this code not reachable in that case? (Reasonable, because invokes aren't tail callable anyhow, right?)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- // It's not safe to eliminate the sign / zero extension of the return value.<br>
- if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex, Attribute::ZExt) ||<br>
- CallerAttrs.hasAttribute(AttributeSet::ReturnIndex, Attribute::SExt))<br>
- return false;<br>
+ bool AllowDifferingSizes = true;<br>
+ if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex, Attribute::ZExt)) {<br>
+ if (!CalleeAttrs.hasAttribute(AttributeSet::ReturnIndex, Attribute::ZExt))<br>
+ return false;<br>
+<br>
+ AllowDifferingSizes = false;<br>
+ } else if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex,<br>
+ Attribute::SExt)) {<br>
+ if (!CalleeAttrs.hasAttribute(AttributeSet::ReturnIndex, Attribute::SExt))<br>
+ return false;<br>
+<br>
+ AllowDifferingSizes = false;<br>
+ }<br></blockquote><div><br></div><div>You're only checking zext and sext. What happen with this:</div><div><br></div><div><div> declare inreg i8 @callee()</div><div> </div><div> define i8 @test() {</div><div> %a = call inreg i8 @callee()</div>
<div> ret i8 %a</div><div> }</div></div><div><br></div><div>We certainly don't want a tail call in this case!</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
const Value *RetVal = Ret->getOperand(0), *CallVal = I;<br>
SmallVector<unsigned, 4> RetPath, CallPath;<br>
@@ -571,7 +578,8 @@<br>
<br>
// Finally, we can check whether the value produced by the tail call at this<br>
// index is compatible with the value we return.<br>
- if (!slotOnlyDiscardsData(RetVal, CallVal, TmpRetPath, TmpCallPath, TLI))<br>
+ if (!slotOnlyDiscardsData(RetVal, CallVal, TmpRetPath, TmpCallPath,<br>
+ AllowDifferingSizes, TLI))<br>
return false;<br>
<br>
CallEmpty = !nextRealType(CallSubTypes, CallPath);<br>
Index: test/CodeGen/X86/sibcall.ll<br>
===================================================================<br>
--- test/CodeGen/X86/sibcall.ll<br>
+++ test/CodeGen/X86/sibcall.ll<br>
@@ -106,10 +106,10 @@<br>
define signext i16 @t8() nounwind ssp {<br>
entry:<br>
; 32-LABEL: t8:<br>
-; 32: calll {{_?}}bar3<br>
+; 32: jmp {{_?}}bar3<br>
<br>
; 64-LABEL: t8:<br>
-; 64: callq {{_?}}bar3<br>
+; 64: jmp {{_?}}bar3<br>
%0 = tail call signext i16 @bar3() nounwind ; <i16> [#uses=1]<br>
ret i16 %0<br>
}<br>
@@ -122,7 +122,7 @@<br>
; 32: calll *<br>
<br>
; 64-LABEL: t9:<br>
-; 64: callq *<br>
+; 64: jmpq *<br>
%0 = bitcast i32 (i32)* %x to i16 (i32)*<br>
%1 = tail call signext i16 %0(i32 0) nounwind<br>
ret i16 %1<br>
Index: test/CodeGen/X86/tail-call-exts.ll<br>
===================================================================<br>
--- /dev/null<br>
+++ test/CodeGen/X86/tail-call-exts.ll<br>
@@ -0,0 +1,39 @@<br>
+; RUN: llc -mtriple=x86_64-apple-darwin -o - %s | FileCheck %s<br>
+<br>
+; Simple case: completely identical returns, even with extensions, shouldn't be<br>
+; a barrier to tail calls.<br>
+declare zeroext i1 @give_bool()<br>
+define zeroext i1 @test_bool() {<br>
+; CHECK-LABEL: test_bool:<br>
+; CHECK: jmp<br>
+ %call = tail call zeroext i1 @give_bool()<br>
+ ret i1 %call<br>
+}<br>
+<br>
+; Here, there's more zero extension to be done between the call and the return,<br>
+; so a tail call is impossible (well, according to current Clang practice<br>
+; anyway. The AMD64 ABI isn't crystal clear on the matter).<br>
+declare zeroext i32 @give_i32()<br>
+define zeroext i8 @test_i32() {<br>
+; CHECK-LABEL: test_i32:<br>
+; CHECK: callq _give_i32<br>
+; CHECK: movzbl %al, %eax<br>
+; CHECK: ret<br>
+<br>
+ %call = tail call zeroext i32 @give_i32()<br>
+ %val = trunc i32 %call to i8<br>
+ ret i8 %val<br>
+}<br>
+<br>
+; Here, one function is zeroext and the other is signext. To the extent that<br>
+; these both mean something they are incompatible so no tail call is possible.<br>
+declare zeroext i16 @give_unsigned_i16()<br>
+define signext i16 @test_incompatible_i16() {<br>
+; CHECK-LABEL: test_incompatible_i16:<br>
+; CHECK: callq _give_unsigned_i16<br>
+; CHECK: cwtl<br>
+; CHECK: ret<br>
+<br>
+ %call = tail call zeroext i16 @give_unsigned_i16()<br>
+ ret i16 %call<br>
+}<br>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div>