[PATCH] Enable inlining of recursive functions.

[Pablo Barrio]

In http://reviews.llvm.org/D6996#110538, @yinma wrote:

> I looked your code, you removed the check in call analyzer and MaxRecursionInl is about inlining history. I would like to know if you set MaxRecursionInl is 0, the inliner will behave exactly like the before without your code?
>
> And MaxRecursionInl is to control the number of history skipped. A different name may be more suitable?


Hello Yin, thank you very much for looking at the code.

When MaxRecursionInl is 0, the code behaves differently at -O3, as stated in paragraph 3 of the Summary. I have benchmarked the patch and it does show some performance improvements/regressions, but overall the change is beneficial. The compile time does not seem to be affected and the code size is only slightly affected.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:437
@@ -431,2 +436,3 @@
 /// indicates an inline history that includes the specified function.
 static bool InlineHistoryIncludes(Function *F, int InlineHistoryID,
+                                  const SmallVectorImpl<std::pair<Function *, int>> &InlineHistory,
----------------
jmolloy wrote:
> What exactly are you trying to do here? You haven't modified the doc comment with the intention.
> 
> What does "allowAtLeast" do? it has "at least" in it but it appears to slurp the first N entries regardless. Also there are coding style violations:
> 
>   * Don't need (and please don't put) brackets around comparison expressions. !=/== binds tighter than &&.
>   * Please don't use iHungarianNotation. "I" or "Skip" would do.
>   * Variables should be UpperCamelCase.
I have added a comment about what "allowAtLeast" (now called SkipFirst) does. The real explanation why I do this is in lines 558-560 of the new patch. This is only a helper function.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:552
@@ +551,3 @@
+        // recursion inlining level
+        if ((MaxRecursionInl == 0) && (Callee == Caller))
+          continue;
----------------
jmolloy wrote:
> Won't this only affect direct recursion? if A -> B -> A, Callee will never equal Caller, right?
Correct. All other cases are handled by InlineHistoryIncludes(). This function cannot handle the case of direct recursion disabled because, in the first level of recursion inlining, the inline history is empty. Therefore, InlineHistoryIncludes() would not encounter any previous inline of the callee and the first level would be inlined.

http://reviews.llvm.org/D6996

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/