[PATCH] D37085: [MachineOutliner] Add missed optimization remarks based off outliner cost model

[Adam Nemet via Phabricator via llvm-commits]

anemet added a comment.

Nice improvements.



================
Comment at: lib/CodeGen/MachineOutliner.cpp:836
+  // remarks an identifier.
+  size_t NumCandidatesEvaluated = 0;
+
----------------
Unused now?


================
Comment at: lib/CodeGen/MachineOutliner.cpp:917-920
+      MachineOptimizationRemarkMissed R(DEBUG_TYPE, "NotOutliningCheaper",
+                                        C.first->getDebugLoc(),
+                                        C.first->getParent());
+      R << NV("StartLoc0", C.first->getDebugLoc()) << ": Did not outline "
----------------
As before, you don't want to output the debugloc here since that is already the debugloc on the remark itself.  This is kind of apparent in your test below but it's even more apparent if you used -pass-remarks-missed=machine-outliner.  You'd have something like:

  remark: machine-outliner-remarks.ll:5:9: machine-outliner-remarks.ll:5:9: Did not outline ...

You want to start at R << "Did not outline " ....

I think it would be a good idea to also run the test below with -pass-remarks-missed=machine-outline and then also match the remark as it is printed on the compiler.  I think the register-spilling testcase should be doing both.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:929-938
+      size_t i = 1;
+      for (size_t e = CandidateClass.size() - 1; i < e; i++) {
+        R << NV("StartLoc" + std::to_string(i),
+                CandidateClass[i].first->getDebugLoc())
+          << ", ";
+      }
+
----------------
Probably nicer:


```
for (size_t i = 1, e = CandidateClass.size(); i < e; i++) {
  R << NV(...)
  if (i != e - 1)
     R << ", ";
}
R << ")";
```


================
Comment at: lib/CodeGen/MachineOutliner.cpp:931
+      for (size_t e = CandidateClass.size() - 1; i < e; i++) {
+        R << NV("StartLoc" + std::to_string(i),
+                CandidateClass[i].first->getDebugLoc())
----------------
Does this work: "StartLoc" + Twine(i)

I would call this OtherStartLoc<i> for easier differentiation form the main loc.


================
Comment at: test/CodeGen/AArch64/machine-outliner-remarks.ll:1
+; RUN: llc %s -enable-machine-outliner -mtriple=aarch64-unknown-unknown -o /dev/null -pass-remarks=machine-outliner -pass-remarks-output=%t.yaml
+; RUN: cat %t.yaml | FileCheck %s -check-prefix=YAML
----------------
Just to repeat what I said above.  Replase -pass-remarks with -pass-remarks-missed (since you're only interested in missed remarks on the stderr) and then check that as well.


================
Comment at: test/CodeGen/AArch64/machine-outliner-remarks.ll:11-18
+; YAML-NEXT:   - String:          ': Did not outline '
+; YAML-NEXT:   - Length:          '2'
+; YAML-NEXT:   - String:          ' instructions. Outlined instruction count ('
+; YAML-NEXT:   - OutliningCost:   '9'
+; YAML-NEXT:   - String:          ')'
+; YAML-NEXT:   - String:          ' >= unoutlined instruction count ('
+; YAML-NEXT:   - NotOutliningCost: '4'
----------------
I am not sure I actually understand diagnostics here.  Shouldn't the non-outlined count (4) be the same as the number of instructions (2)?


https://reviews.llvm.org/D37085