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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

Some minor changes are required to make it more clear what this does.



================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:81-82
+BlockFrequencyRatioThreshold("block-freq-ratio-threshold",
+		             cl::desc("Block frequency ratio threshold to "
+			              "disable instruction hoisting"),
+		             cl::init(100), cl::Hidden);
----------------
lebedev.ri wrote:
> It isn't really obvious whether larger value relaxes this check or the other way around
We should probably be more forthcoming with this description so that it is clear to the user.
Perhaps:
```
Do not hoist instructions if the target block is N times hotter than the source.
```

Also, we should add a comment here along the lines of:
```
// The default threshold of 100 (i.e. if target block is 100 times hotter)
// is based on empirical data on a single target and is subject to tuning.
```


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:85
+
+enum NotHoistDueToBlockHotnessType { Disable, Enable_PGO, Enable };
+
----------------
Similarly to below, the naming is a bit strange. Perhaps:
```
enum DisableHoistingToHotterBlocks { None, PGO, All };
```
or
```
enum DisableHoistingToHotterBlocks { None, PGO_Only, Static };
```
(although I prefer the former).


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:88
+static cl::opt<NotHoistDueToBlockHotnessType>
+NotHoistDueToBlockHotness("not-hoist-due-to-block-hotness",
+                          cl::desc("Disable instruction hoist due to block "
----------------
The name sounds very strange. Perhaps:
```
DisableHoistingToHotterBlocks("disable-hoisting-to-hotter-blocks",
```


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:112
+STATISTIC(NumNotHoistedDueToHotness,
+          "Number of instructions not hoisted due to block frequency hotness");
 
----------------
"block frequency hotness" sounds strange. Please use either "hotness" or "block frequency".


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:275
 
+    bool IsHoistingFromColdToHotBlock(MachineBasicBlock *BB,
+                                      MachineBasicBlock *Preheader);
----------------
I think it is clearer to just use `SrcBlock` and `TgtBlock` for the names of the blocks.


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:1471
+  MachineBasicBlock *BB = MI->getParent();
+  bool hasProfileData = BB->getParent()->getFunction().hasProfileData();
+
----------------
Presumably this function is called many times for the same function and we are querying the function for profile data every time. Would it be possible to just have a class member flag for whether the function has profile data or not?


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:1581
+  if (!SrcBF)
+    return true;
+
----------------
We don't want to increment the statistic here?

Honestly, I don't think we should be incrementing the statistic in this function at all - this is just a query for whether or not this is profitable. Strictly speaking, the increment to the statistic really belongs in the function that would actually do the hoisting (i.e. `Hoist()`).


================
Comment at: llvm/lib/CodeGen/MachineLICM.cpp:1585
+  // Compare the block frequency ratio with the threshold
+  if (Ratio > BlockFrequencyRatioThreshold) {
+    ++NumNotHoistedDueToHotness;
----------------
If we move the update to the statistic out of this function, this can probably just be changed to:
`return Ratio > BlockFrequencyRatioThreshold;`


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

https://reviews.llvm.org/D63676