[PATCH] Enable inlining of recursive functions.

[James Molloy]

Hi Pablo,

Thanks for working on this! I have comments inline. But in general, it looks like this patch just allows recursive functions to be inlined, it doesn't change the heuristic calculation for them.

My impression from previous threads (Chandler will vociferiously declaim me if I misremember, I'm sure) is that Chandler expressed a wish that recursive inlining be treated heuristically more like loop unrolling, as they are somewhat equivalent. Do you have a comment on that?

Cheers,

James


================
Comment at: lib/Analysis/IPA/InlineCost.cpp:913
@@ -920,3 +912,3 @@
     // If the visit this instruction detected an uninlinable pattern, abort.
-    if (IsRecursiveCall || ExposesReturnsTwice || HasDynamicAlloca ||
+    if (ExposesReturnsTwice || HasDynamicAlloca ||
         HasIndirectBr)
----------------
The continuation line can now fit on one line.

================
Comment at: lib/Analysis/IPA/InlineCost.cpp:1141
@@ -1148,3 +1140,3 @@
     if (!analyzeBlock(BB, EphValues)) {
-      if (IsRecursiveCall || ExposesReturnsTwice || HasDynamicAlloca ||
+      if (ExposesReturnsTwice || HasDynamicAlloca ||
           HasIndirectBr)
----------------
Remove line break

================
Comment at: lib/Transforms/IPO/Inliner.cpp:64
@@ -63,1 +63,3 @@
 
+// Control the level of recursion inlining.
+static cl::opt<int>
----------------
Could you please be more explicit about what exactly this knob does?

  * What does "levels" mean? for example: B -> C -> B -> C - is that 2 levels or 4 levels?
  * Explicitly put the default in the description message
  * I assume this doesn't alter heuristics at all?

================
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,
----------------
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.

================
Comment at: lib/Transforms/IPO/Inliner.cpp:552
@@ +551,3 @@
+        // recursion inlining level
+        if ((MaxRecursionInl == 0) && (Callee == Caller))
+          continue;
----------------
Won't this only affect direct recursion? if A -> B -> A, Callee will never equal Caller, right?

http://reviews.llvm.org/D6996

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