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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:391
+  // If highly predictable, branch form is more profitable.
+  if (isSelectHighlyPredictable(SI)) {
+    ++NumSelectConvertedHighPred;
----------------
If the SI is in a cold basic block (as determined by profile summary), it may not be worth converting.


================
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);
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:432
+      ColdI = dyn_cast<Instruction>(SI->getFalseValue());
+    if (ColdI) {
+      SmallVector<Instruction *, 2> ColdSlice;
----------------
assert ColdI != NULL?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:436
+      for (auto *ColdII : ColdSlice)
+        if (isExpensiveInst(ColdII))
+          return true;
----------------
what if all defs in the chain are cheap, but they add up to be expensive?


================
Comment at: llvm/lib/CodeGen/SelectOptimize.cpp:447
+// the source instruction.
+void SelectOptimize::getOneUseSlice(Instruction *I,
+                                    SmallVector<Instruction *, 2> &Slice) {
----------------
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.


================
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;
----------------
Is this possible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120231