[PATCH] D50435: [Inliner] Attribute callsites with inline remarks

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

A few comments/questions.



================
Comment at: lib/Transforms/IPO/Inliner.cpp:119
+    InlineRemarkAttribute("inline-remark-attribute", cl::init(false),
+                           cl::Hidden);
+
----------------
Add a cl::desc for option.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:507
+static void setInlineRemark(CallSite &CS, StringRef message) {
+  if (InlineRemarkAttribute) {
+    Attribute attr = Attribute::get(CS->getContext(), "inline-remark", message);
----------------
LLVM coding style prefers early return to code nested in if body.


================
Comment at: lib/Transforms/IPO/Inliner.cpp:646
+      if (!OIC.getValue()) {
+        setInlineRemark(CS, inlineCostStr(*OIC)); // OIC explains
         continue;
----------------
The comment here isn't very meaningful. Where does the OIC explain this? Please expand/replace comment (here and at line 1034).


================
Comment at: lib/Transforms/IPO/Inliner.cpp:673
         if (!IR) {
+          setInlineRemark(CS, std::string(IR) + "; " + inlineCostStr(*OIC));
           ORE.emit([&]() {
----------------
I'm not sure what this ends up looking like - can you add a case with this string to your test?


================
Comment at: lib/Transforms/IPO/Inliner.cpp:1053
       if (!IR) {
+        setInlineRemark(CS, std::string(IR.message) + "; " + inlineCostStr(*OIC));
         ORE.emit([&]() {
----------------
Why IR.message here but only IR online 673?


https://reviews.llvm.org/D50435