[PATCH] D19398: Second for inlining report

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: include/llvm/Analysis/InlineCost.h:48
@@ +47,3 @@
+/// beginning with Inlr are reasons FOR inlining.  Those with names
+/// beginning with Ninlr are reasons for NOT inlining.
+///
----------------
Fundamentally, I don't think this is the right abstraction.  You have a set of reasons that you're trying to squeeze down into a single value.  We're probably better off exposing all the contributing reasons to the frontend with a ranking to indicate how important they are.

The feedback on the earlier patch seems to be leading in the same direction.  I'm not going to review this patch further until that part has been worked out.  

================
Comment at: include/llvm/Analysis/InlineCost.h:55
@@ +54,3 @@
+///
+typedef enum {
+   InlrFirst, // Just a marker placed before the first inlining reason
----------------
enum X {

================
Comment at: include/llvm/Analysis/InlineCost.h:58
@@ +57,3 @@
+   InlrNoReason,
+   InlrAlwaysInline,
+   InlrSingleLocalCall,
----------------
style: please don't be concise over understandable.

================
Comment at: include/llvm/Analysis/InlineCost.h:149
@@ -72,2 +148,3 @@
+  }
   static InlineCost getAlways() {
     return InlineCost(AlwaysInlineCost, 0);
----------------
Having versions without reasons are suspicious.

================
Comment at: lib/Analysis/InlineCost.cpp:221
@@ -211,3 +220,3 @@
 
-  bool analyzeCall(CallSite CS);
+  bool analyzeCall(CallSite CS, InlineReason* Reason);
 
----------------
You probably want a reference here.

================
Comment at: lib/Analysis/InlineCost.cpp:960
@@ -950,3 +959,3 @@
   CallAnalyzer CA(TTI, ACT, *F, InlineConstants::IndirectCallThreshold, CS);
-  if (CA.analyzeCall(CS)) {
+  if (CA.analyzeCall(CS, nullptr)) {
     // We were able to inline the indirect call! Subtract the cost from the
----------------
Probably better to populate a local variable and discard it.  Optional params via pointers tend to be very error prone.

================
Comment at: lib/Analysis/InlineCost.cpp:1394
@@ +1393,3 @@
+      if (ExposesReturnsTwice) { 
+        *ReasonAddr = NinlrReturnsTwice;
+        return false; 
----------------
The long chain of modifications hints that maybe we should just be returning the reason we didn't inline and converting that into a boolean once.  


http://reviews.llvm.org/D19398