[PATCH] D63676: Disable hosting MI to hotter basic blocks

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

With the minor comments addressed, this LGTM.
However before you update and commit, it would be good to hear from at least @lebedev.ri about whether his requests were satisfactorily met.



================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:87
+
+enum DisableHoistingToHotterBlocksType { NONE, PGO, All };
+
----------------
It is common practice to differentiate enumerators in one of two ways:
1. Prefix each enumerator with an acronym of the enumeration
2. Use scoped enumerations

While we're at it, you might as well simplify the naming to just `UseBFI`
Examples (with simplified name):
1. `enum UseBFI { UseBFI_None, UseBFI_PGO, UseBFI_All }`
2. `enum class UseBFI { None, PGO, All }`

My preference is 2.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:91
+DisableHoistingToHotterBlocks("disable-hoisting-to-hotter-blocks",
+                              cl::desc("Disable hosting instructions to"
+                              " hotter blocks"),
----------------
Please replace all uses of the word `hosting` with the word `hoisting` since we are changing the decisions about **hoisting** instructions out of loops. In addition to correcting it in code, please correct it in the description, title, commit message, etc.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:126
     bool PreRegAlloc;
+    bool hasProfileData;
 
----------------
Nit: naming convention (variables still start with uppercase letters).


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:278
 
+    bool IsHoistingFromColdToHotBlock(MachineBasicBlock *SrcBlock,
+                                      MachineBasicBlock *TgtBlock);
----------------
Nit: naming convention - functions start with a lowercase letter. And since this doesn't do any hoisting, I think a simpler name would be more appropriate. Perhaps 
```
// Is the target basic block at least "BlockFrequencyRatioThreshold" times
// hotter than the source basic block.
bool isTgtHotterThanSrc(MachineBasicBlock *SrcBlock,
                        MachineBasicBlock *TgtBlock);
```


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:1578
+bool MachineLICMBase::IsHoistingFromColdToHotBlock(MachineBasicBlock *SrcBlock,
+    MachineBasicBlock *TgtBlock) {
+  // Parse source and target basic block frequency from MBFI
----------------
This indentation is suspicious. If that's where `clang-format` put this line, so be it, but it just seems odd.


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

https://reviews.llvm.org/D63676