[PATCH] Allow reasonable zeroext/signext tail calls

Nick Lewycky nlewycky at google.com
Tue Aug 6 18:41:05 PDT 2013


On 6 August 2013 06:09, Tim Northover <t.p.northover at gmail.com> wrote:

> Hi nicholas,
>
> As in PR16742, we should probably be able to handle tail calls like:
>     bool g(void);
>     bool f() { return g(); }
>
> 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.
>
> This patch implements that logic:
> + If the caller (@f) has no extension attribute, anything goes, as before
> -- the higher bits are undef.
> + If the caller has zeroext/signext and the callee has different, then
> things will go wrong.
> + Otherwise, the conventions match so make sure the bit-width is the same
> in each case.
>
> http://llvm-reviews.chandlerc.com/D1299
>
> Files:
>   lib/CodeGen/Analysis.cpp
>   test/CodeGen/X86/sibcall.ll
>   test/CodeGen/X86/tail-call-exts.ll
>
> Index: lib/CodeGen/Analysis.cpp
> ===================================================================
> --- lib/CodeGen/Analysis.cpp
> +++ lib/CodeGen/Analysis.cpp
> @@ -320,6 +320,7 @@
>  static bool slotOnlyDiscardsData(const Value *RetVal, const Value
> *CallVal,
>                                   SmallVectorImpl<unsigned> &RetIndices,
>                                   SmallVectorImpl<unsigned> &CallIndices,
> +                                 bool AllowDifferingSizes,
>                                   const TargetLoweringBase &TLI) {
>
>    // Trace the sub-value needed by the return value as far back up the
> graph as
> @@ -350,7 +351,8 @@
>    // all the bits that are needed by the "ret" have been provided by the
> "tail
>    // call". FIXME: with sufficiently cunning bit-tracking, we could look
> through
>    // extensions too.
> -  if (BitsProvided < BitsRequired)
> +  if (BitsProvided < BitsRequired ||
> +      (!AllowDifferingSizes && BitsProvided != BitsRequired))
>      return false;
>
>    return true;
> @@ -517,20 +519,25 @@
>    // return type is.
>    if (isa<UndefValue>(Ret->getOperand(0))) return true;
>
> -  // Conservatively require the attributes of the call to match those of
> -  // the return. Ignore noalias because it doesn't affect the call
> sequence.
> -  const Function *F = ExitBB->getParent();
> -  AttributeSet CallerAttrs = F->getAttributes();
> -  if (AttrBuilder(CallerAttrs, AttributeSet::ReturnIndex).
> -        removeAttribute(Attribute::NoAlias) !=
> -      AttrBuilder(CallerAttrs, AttributeSet::ReturnIndex).
> -        removeAttribute(Attribute::NoAlias))
> -    return false;
> +  // If the ABI expects the caller to do some extension of the result
> before
> +  // return, then in general we shouldn't allow a truncate between the
> call-site
> +  // and the ret.
> +  AttributeSet CallerAttrs = ExitBB->getParent()->getAttributes();
> +  AttributeSet CalleeAttrs = cast<CallInst>(I)->getAttributes();
>

What if it's an InvokeInst? Or is this code not reachable in that case?
(Reasonable, because invokes aren't tail callable anyhow, right?)

-  // It's not safe to eliminate the sign / zero extension of the return
> value.
> -  if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex,
> Attribute::ZExt) ||
> -      CallerAttrs.hasAttribute(AttributeSet::ReturnIndex,
> Attribute::SExt))
> -    return false;
> +  bool AllowDifferingSizes = true;
> +  if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex,
> Attribute::ZExt)) {
> +    if (!CalleeAttrs.hasAttribute(AttributeSet::ReturnIndex,
> Attribute::ZExt))
> +      return false;
> +
> +    AllowDifferingSizes = false;
> +  } else if (CallerAttrs.hasAttribute(AttributeSet::ReturnIndex,
> +                                      Attribute::SExt)) {
> +    if (!CalleeAttrs.hasAttribute(AttributeSet::ReturnIndex,
> Attribute::SExt))
> +      return false;
> +
> +    AllowDifferingSizes = false;
> +  }
>

You're only checking zext and sext. What happen with this:

  declare inreg i8 @callee()

  define i8 @test() {
    %a = call inreg i8 @callee()
    ret i8 %a
  }

We certainly don't want a tail call in this case!

Nick


>
>    const Value *RetVal = Ret->getOperand(0), *CallVal = I;
>    SmallVector<unsigned, 4> RetPath, CallPath;
> @@ -571,7 +578,8 @@
>
>      // Finally, we can check whether the value produced by the tail call
> at this
>      // index is compatible with the value we return.
> -    if (!slotOnlyDiscardsData(RetVal, CallVal, TmpRetPath, TmpCallPath,
> TLI))
> +    if (!slotOnlyDiscardsData(RetVal, CallVal, TmpRetPath, TmpCallPath,
> +                              AllowDifferingSizes, TLI))
>        return false;
>
>      CallEmpty  = !nextRealType(CallSubTypes, CallPath);
> Index: test/CodeGen/X86/sibcall.ll
> ===================================================================
> --- test/CodeGen/X86/sibcall.ll
> +++ test/CodeGen/X86/sibcall.ll
> @@ -106,10 +106,10 @@
>  define signext i16 @t8() nounwind ssp {
>  entry:
>  ; 32-LABEL: t8:
> -; 32: calll {{_?}}bar3
> +; 32: jmp {{_?}}bar3
>
>  ; 64-LABEL: t8:
> -; 64: callq {{_?}}bar3
> +; 64: jmp {{_?}}bar3
>    %0 = tail call signext i16 @bar3() nounwind      ; <i16> [#uses=1]
>    ret i16 %0
>  }
> @@ -122,7 +122,7 @@
>  ; 32: calll *
>
>  ; 64-LABEL: t9:
> -; 64: callq *
> +; 64: jmpq *
>    %0 = bitcast i32 (i32)* %x to i16 (i32)*
>    %1 = tail call signext i16 %0(i32 0) nounwind
>    ret i16 %1
> Index: test/CodeGen/X86/tail-call-exts.ll
> ===================================================================
> --- /dev/null
> +++ test/CodeGen/X86/tail-call-exts.ll
> @@ -0,0 +1,39 @@
> +; RUN: llc -mtriple=x86_64-apple-darwin -o - %s | FileCheck %s
> +
> +; Simple case: completely identical returns, even with extensions,
> shouldn't be
> +; a barrier to tail calls.
> +declare zeroext i1 @give_bool()
> +define zeroext i1 @test_bool() {
> +; CHECK-LABEL: test_bool:
> +; CHECK: jmp
> +  %call = tail call zeroext i1 @give_bool()
> +  ret i1 %call
> +}
> +
> +; Here, there's more zero extension to be done between the call and the
> return,
> +; so a tail call is impossible (well, according to current Clang practice
> +; anyway. The AMD64 ABI isn't crystal clear on the matter).
> +declare zeroext i32 @give_i32()
> +define zeroext i8 @test_i32() {
> +; CHECK-LABEL: test_i32:
> +; CHECK: callq _give_i32
> +; CHECK: movzbl %al, %eax
> +; CHECK: ret
> +
> +  %call = tail call zeroext i32 @give_i32()
> +  %val = trunc i32 %call to i8
> +  ret i8 %val
> +}
> +
> +; Here, one function is zeroext and the other is signext. To the extent
> that
> +; these both mean something they are incompatible so no tail call is
> possible.
> +declare zeroext i16 @give_unsigned_i16()
> +define signext i16 @test_incompatible_i16() {
> +; CHECK-LABEL: test_incompatible_i16:
> +; CHECK: callq _give_unsigned_i16
> +; CHECK: cwtl
> +; CHECK: ret
> +
> +  %call = tail call zeroext i16 @give_unsigned_i16()
> +  ret i16 %call
> +}
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130806/efcf860d/attachment.html>


More information about the llvm-commits mailing list