[PATCH] D120231: [SelectOpti][3/5] Base Heuristics

[Sotiris Apostolakis via Phabricator via llvm-commits]

apostolakis added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:391
+  // If highly predictable, branch form is more profitable.
+  if (isSelectHighlyPredictable(SI)) {
+    ++NumSelectConvertedHighPred;
----------------
davidxl wrote:
> If the SI is in a cold basic block (as determined by profile summary), it may not be worth converting.
That's true. 
Selects in cold functions will not be considered for conversion since this is tested earlier by calling llvm::shouldOptimizeForSize. But cold basic blocks within a non-cold function are not checked. Added a check for coldness at the basic block level to prevent any conversion.  


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:416
+  uint64_t TrueWeight, FalseWeight;
+  if (ASI.front()->extractProfMetadata(TrueWeight, FalseWeight)) {
+    uint64_t Min = std::min(TrueWeight, FalseWeight);
----------------
davidxl wrote:
> When profile data is available, but the select instruction does not have a meta data attached, we may want to emit a missed optimization warning.
Sure, added.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:432
+      ColdI = dyn_cast<Instruction>(SI->getFalseValue());
+    if (ColdI) {
+      SmallVector<Instruction *, 2> ColdSlice;
----------------
davidxl wrote:
> assert ColdI != NULL?
The value operands of the select instructions are not necessarily instructions, could be for example, a function argument.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:436
+      for (auto *ColdII : ColdSlice)
+        if (isExpensiveInst(ColdII))
+          return true;
----------------
davidxl wrote:
> what if all defs in the chain are cheap, but they add up to be expensive?
Yes, many cheap ones could amount to be as costly as one expensive one. Tweaked the heuristic to account for that.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:447
+// the source instruction.
+void SelectOptimize::getOneUseSlice(Instruction *I,
+                                    SmallVector<Instruction *, 2> &Slice) {
----------------
davidxl wrote:
> Perhaps make the name of the method more general to match its intention: get instructions that can be wrapped into the cold branch when converted to control flow. Also make it clear that current heuristic chooses only oneUse defs.
Good point. Changed the function name and the comments. 


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:466
+    // instruction (i.e., avoid colder code regions of the dependence slice).
+    if (BFI->getBlockFreq(II->getParent()) < BFI->getBlockFreq(I->getParent()))
+      continue;
----------------
davidxl wrote:
> Is this possible?
This branch can go both ways:

Example where freq(II) < freq(I):
```
II = ...
for () {
    I = II  + ...
    x = select c, I, ...
}
```

Example where freq(II) > freq(I):
```
for () {
   II = ...
}
I = II  + ...
x = select c, I, ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120231