<div class="gmail_quote">On Sat, Mar 31, 2012 at 3:34 PM, Matt Beaumont-Gay <span dir="ltr"><<a href="mailto:matthewbg@google.com">matthewbg@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sat, Mar 31, 2012 at 06:17, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> --- llvm/trunk/lib/Transforms/IPO/InlineAlways.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/IPO/InlineAlways.cpp Sat Mar 31 08:17:18 2012<br>
> @@ -32,7 +32,6 @@<br>
><br>
>   // AlwaysInliner only inlines functions that are mark as "always inline".<br>
>   class AlwaysInliner : public Inliner {<br>
> -    InlineCostAnalyzer CA;<br>
>   public:<br>
>     // Use extremely low threshold.<br>
>     AlwaysInliner() : Inliner(ID, -2000000000, /*InsertLifetime*/true) {<br>
> @@ -43,24 +42,7 @@<br>
>       initializeAlwaysInlinerPass(*PassRegistry::getPassRegistry());<br>
>     }<br>
>     static char ID; // Pass identification, replacement for typeid<br>
> -    InlineCost getInlineCost(CallSite CS) {<br>
> -      Function *Callee = CS.getCalledFunction();<br>
> -      // We assume indirect calls aren't calling an always-inline function.<br>
> -      if (!Callee) return InlineCost::getNever();<br>
> -<br>
> -      // We can't inline calls to external functions.<br>
> -      // FIXME: We shouldn't even get here.<br>
> -      if (Callee->isDeclaration()) return InlineCost::getNever();<br>
> -<br>
> -      // Return never for anything not marked as always inline.<br>
> -      if (!Callee->hasFnAttr(Attribute::AlwaysInline))<br>
> -        return InlineCost::getNever();<br>
> -<br>
> -      // We still have to check the inline cost in case there are reasons to<br>
> -      // not inline which trump the always-inline attribute such as setjmp and<br>
> -      // indirectbr.<br>
> -      return CA.getInlineCost(CS, getInlineThreshold(CS));<br>
> -    }<br>
> +    virtual InlineCost getInlineCost(CallSite CS);<br>
>     virtual bool doFinalization(CallGraph &CG) {<br>
>       return removeDeadFunctions(CG, /*AlwaysInlineOnly=*/true);<br>
>     }<br>
> @@ -81,9 +63,70 @@<br>
>   return new AlwaysInliner(InsertLifetime);<br>
>  }<br>
><br>
> +/// \brief Minimal filter to detect invalid constructs for inlining.<br>
> +static bool isInlineViable(Function &F) {<br>
> +  bool ReturnsTwice = F.hasFnAttr(Attribute::ReturnsTwice);<br>
> +  for (Function::iterator BI = F.begin(), BE = F.end(); BI != BE; ++BI) {<br>
> +    // Disallow inlining of functions which contain an indirect branch.<br>
> +    if (isa<IndirectBrInst>(BI->getTerminator()))<br>
> +      return false;<br>
> +<br>
> +    for (BasicBlock::iterator II = BI->begin(), IE = BI->end(); II != IE;<br>
> +         ++II) {<br>
> +      CallSite CS(II);<br>
> +      if (!CS)<br>
> +        continue;<br>
> +<br>
> +      // Disallow recursive calls.<br>
> +      if (&F == CS.getCalledFunction())<br>
> +        return false;<br>
> +<br>
> +      // Disallow calls which expose returns-twice to a function not previously<br>
> +      // attributed as such.<br>
> +      if (ReturnsTwice && CS.isCall() &&<br>
<br>
</div></div>Er, should be "if (!ReturnsTwice && ...)", no?<br></blockquote><div><br></div><div>Rather! Thanks for the catch!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, tests?<br></blockquote><div><br></div><div>Well, I had *thought* that this was nicely covered by existing tests, as this change was supposed to have no observable behavior change, just be ridiculously simpler... Yet clearly this functionality is hardly tested at all. I'll try to get enough tests to catch this bug...</div>
</div>