[PATCH] D61665: [TailCall] Disable tail call if the callee function contain __builtin_frame_address or __builtin_return_address

Wed May 8 18:52:01 PDT 2019

shiva0217 abandoned this revision.
shiva0217 added a subscriber: eli.friedman.
shiva0217 added a comment.

In D61665#1495723 <https://reviews.llvm.org/D61665#1495723>, @hfinkel wrote:

> In D61665#1495701 <https://reviews.llvm.org/D61665#1495701>, @efriedma wrote:
>
> > I have a few concerns here:
> >
> > 1. Looping over every instruction in a function is expensive, and makes any pass which checks this for every call in a function take quadratic time overall.
>

It may too expansive, using function attribute as Hal's comment would be a better approach.

>> 2. You can't inspect the body of a function pointer, or a function in a different translation unit, so we can't make this work consistently.

Yes, we can't detect the function in a different translation unit or pointed by a function pointer.

>> 3. Even in the same translation unit, how do we "preserve" the behavior for values greater than 1?

Yes, disabling tail call can only preserve the stack in depth 1, it may have other optimizations change the behavior of the depth greater than one.

>> 
>> 
>>  ------
>> 
>> I'd prefer to just leave the current behavior if it isn't causing any practical problems.  The user can always use -fno-optimize-sibling-calls if their codebase needs it for some reason.
> 
> If we do wish to make our "best effort" contain more effort, I think that we'd want to do this during function-attribute inference - there we can iterate over the call graph and add some inhibiting attributes. That having been said, if the only use case we have for this is matching some portion of GCC's heuristic for the purpose of making their test case pass, I'm not sure that this is worthwhile.

I think the consensus is to leave the current behavior. Thanks @eli.friedman pointed out the cases the patch can't cover and the better approach suggested by @hfinkel if we would like to do so.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61665/new/

https://reviews.llvm.org/D61665