[PATCH] D150464: [FuncSpec] Improve the accuracy of the cost model.

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:586
   // instructions in the function.
-  return Metrics.NumInsts * InlineConstants::getInstrCost();
+  return Metrics.NumInsts;
 }
----------------
Is this change necessary? Or what is the intention? It looks not good to me at the first sight.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:593
+  Cost Bonus = 0;
+  if (I->getCondition() == V) {
+    Function *F = I->getFunction();
----------------
We prefer code style with shorter ident.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:609-635
+    while (!WorkList.empty()) {
+      BasicBlock *BB = WorkList.pop_back_val();
+
+      uint64_t Weight = BFI.getBlockFreq(BB).getFrequency() /
+                        BFI.getEntryFreq();
+      if (!Weight)
+        continue;
----------------
Oh, good idea. But I only understand what you're trying to do after reading the codes. A comment here will be helpful.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:640-642
+Cost FunctionSpecializer::estimateBranchInst(BranchInst *I, Value *V,
+                                             Constant *C,
+                                             ConstMap &KnownConstants) {
----------------
This looks similar with `estimateSwitchInst`. Can we try to merge them?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:644
+  Cost Bonus = 0;
+  if (I->getCondition() == V) {
+    Function *F = I->getFunction();
----------------
Ditto.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:686
 
+Cost FunctionSpecializer::getUserBonus(Instruction *User, Value *Use,
+                                       Constant *C, ConstMap &KnownConstants) {
----------------
Maybe we can make it look prettier by using `InstVisitor` for such code patterns.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:690-692
+  if (auto *I = dyn_cast<SwitchInst>(User)) {
+    return estimateSwitchInst(I, Use, cast<ConstantInt>(C), KnownConstants);
+  } else if (auto *I = dyn_cast<BranchInst>(User)) {
----------------
We don't like else-after-return pattern.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:718
+      auto Ops = ArrayRef(Operands.begin() + 1, Operands.end());
+      C = ConstantExpr::getGetElementPtr(I->getSourceElementType(), Ptr, Ops);
+    }
----------------
Constant expression is going to be deprecated: https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179. Let's not add more constant expression.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:724-726
+      if (!C)
+        if (auto It = KnownConstants.find(V); It != KnownConstants.end())
+          C = It->second;
----------------
Such code pattern looks **a little bit** dirty. Let's try to wrap it into a function.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:768
+
+  if (C) {
+    KnownConstants.insert({User, C});
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150464