[PATCH] D99876: [GreedyRA] Add some stats to remark emitter for Greedy RA.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:3121
+
+  ORE->emit([&]() {
+    float TotalMemWeight = 0.0f;
----------------
Please move the majority of the computation logic outside the ORE emit lambda.  Leave only the output formatting there.  (Just to be consistent with existing code.)


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:3133
+            TII->getNonFoldableRange(MI);
+        if (any_of(MI.operands(), [&](MachineOperand &MO) {
+              return MO.isFI() && MFI.isSpillSlotObjectIndex(MO.getIndex()) &&
----------------
You'll need to restructure this a bit on the changes I suggested for the review adding getNonFoldableRange.

I'll mention that I found the current structure quite confusing.  I'd suggest adjusting naming to be clear that what you're interested in is zero cost folding, not whether these is a fold.  To the point where it might be useful to have stasts to track each separately.  


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:3156
+                                        DebugLoc(), &MF->front());
+    R << NV("Total Spill Count", TotalMemCount);
+    R << NV("Total Spill Cost", TotalMemWeight);
----------------
You need to find some alternate naming here.  You're not counting spills, you're counting usages of spills.

You might want to count "folded reloads" and "folded spills" separately?

As an aside, extending the existing reportNumberOfSplillsReloads to report a function level summary as well as the loop level detail would seem quite helpful.  

Actually, the more I look at this, the more I think what you actually want is to extend the existing reportNumberOfSplillsReloads logic after extending it the function level.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99876/new/

https://reviews.llvm.org/D99876