[PATCH] D36054: Emit only A Single Opt Remark When Inlining

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

Thanks for that review, I'll get onto doing the changes you've asked for.



================
Comment at: lib/Transforms/IPO/Inliner.cpp:387-389
+    // IC does not bool() to false, so get an InlineCost that will.
+    // This will not be inspected to make an error message.
+    return InlineCost::getNever();
----------------
anemet wrote:
> I am fine with this.  Alternatively we could change the return type to Optional<InlineCost> and return None here and check (!IC || !*IC) in the caller but that seems like an overkill.
I think `Optional<InlineCost>` might be a better solution than a magic value that happens to do what we want. I'll see what changing to that does. 


================
Comment at: lib/Transforms/IPO/Inliner.cpp:577-581
+        if (IC.isAlways()) {
+          ORE.emit(OptimizationRemark(DEBUG_TYPE, "AlwaysInline", DLoc, Block)
+                   << NV("Callee", Callee) << " inlined into "
+                   << NV("Caller", Caller) << " with cost=always");
+        } else {
----------------
anemet wrote:
> No {} around a single statement.
Sure. I thought I had run clang-format, but it evidently didn't catch this. I'll do it manually.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:879-882
+      InlineCost IC = shouldInline(CS, GetInlineCost, ORE);
       // Check whether we want to inline this callsite.
-      if (!shouldInline(CS, GetInlineCost, ORE))
+      if (!IC)
         continue;
----------------
anemet wrote:
> Isn't this the path via the new PM?  Don't you need to emit the remarks for the successful inlining here as well?
> 
> Should probably add a test for this if I am right ;).
Ok, I didn't understand why there were two implementations, one for each pass makes more sense.

I need to fix that bug with the path option, which might need to land first, so that tests start failing. 


https://reviews.llvm.org/D36054