<div dir="ltr">On 9 August 2013 05:19, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Thanks Nick, you're quite right about not ignoring the other attributes. I looked through the list but missed that one.<br>
<br>
I think a conservative approach of bailing on anything we don't understand is probably right for now. Who knows what other attributes will be added in future? I've done that and added tests both ways for the "inreg" attribute.<br>
<br>
I think the invokes aren't an issue; as you say, they can't be in tail position and can't have the "tail" specifier that triggers this call.<br></blockquote><div><br></div><div>LGTM!</div><div><br>
</div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi nicholas,<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1299" target="_blank">http://llvm-reviews.chandlerc.com/D1299</a><br>
<br>
CHANGE SINCE LAST DIFF<br>
<a href="http://llvm-reviews.chandlerc.com/D1299?vs=3224&id=3315#toc" target="_blank">http://llvm-reviews.chandlerc.com/D1299?vs=3224&id=3315#toc</a><br>
<br>
Files:<br>
lib/CodeGen/Analysis.cpp<br>
test/CodeGen/X86/sibcall.ll<br>
test/CodeGen/X86/tail-call-attrs.ll<br>
<div class="im"><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>
</div>@@ -517,19 +519,38 @@<br>
<div class="im"> // 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>
</div>+ // Make sure the attributes attached to each return are compatible.<br>
+ AttrBuilder CallerAttrs(ExitBB->getParent()->getAttributes(),<br>
+ AttributeSet::ReturnIndex);<br>
+ AttrBuilder CalleeAttrs(cast<CallInst>(I)->getAttributes(),<br>
+ AttributeSet::ReturnIndex);<br>
+<br>
+ // Noalias is completely benign as far as calling convention goes, it<br>
+ // shouldn't affect whether the call is a tail call.<br>
+ CallerAttrs = CallerAttrs.removeAttribute(Attribute::NoAlias);<br>
+ CalleeAttrs = CalleeAttrs.removeAttribute(Attribute::NoAlias);<br>
<div class="im">+<br>
+ bool AllowDifferingSizes = true;<br>
</div>+ if (CallerAttrs.contains(Attribute::ZExt)) {<br>
+ if (!CalleeAttrs.contains(Attribute::ZExt))<br>
<div class="im">+ return false;<br>
+<br>
+ AllowDifferingSizes = false;<br>
</div>+ CallerAttrs.removeAttribute(Attribute::ZExt);<br>
+ CalleeAttrs.removeAttribute(Attribute::ZExt);<br>
+ } else if (CallerAttrs.contains(Attribute::SExt)) {<br>
+ if (!CalleeAttrs.contains(Attribute::SExt))<br>
<div class="im">+ return false;<br>
+<br>
+ AllowDifferingSizes = false;<br>
</div>+ CallerAttrs.removeAttribute(Attribute::SExt);<br>
+ CalleeAttrs.removeAttribute(Attribute::SExt);<br>
+ }<br>
<div class="im"><br>
- // 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>
</div>+ // If they're still different, there's some facet we don't understand<br>
+ // (currently only "inreg", but in future who knows). It may be OK but the<br>
+ // only safe option is to reject the tail call.<br>
+ if (CallerAttrs != CalleeAttrs)<br>
return false;<br>
<div class="im"><br>
const Value *RetVal = Ret->getOperand(0), *CallVal = I;<br>
</div>@@ -571,7 +592,8 @@<br>
<div><div class="h5"><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>
</div></div>Index: test/CodeGen/X86/tail-call-attrs.ll<br>
===================================================================<br>
--- /dev/null<br>
+++ test/CodeGen/X86/tail-call-attrs.ll<br>
@@ -0,0 +1,56 @@<br>
<div><div class="h5">+; 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>
</div></div>+<br>
+declare inreg i32 @give_i32_inreg()<br>
+define i32 @test_inreg_to_normal() {<br>
+; CHECK-LABEL: test_inreg_to_normal:<br>
+; CHECK: callq _give_i32_inreg<br>
+; CHECK: ret<br>
+ %val = tail call inreg i32 @give_i32_inreg()<br>
+ ret i32 %val<br>
+}<br>
+<br>
+define inreg i32 @test_normal_to_inreg() {<br>
+; CHECK-LABEL: test_normal_to_inreg:<br>
+; CHECK: callq _give_i32<br>
+; CHECK: ret<br>
+ %val = tail call i32 @give_i32()<br>
+ ret i32 %val<br>
+}<br>
\ No newline at end of file<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>