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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added a subscriber: mingmingl.
labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:586
   // instructions in the function.
-  return Metrics.NumInsts * InlineConstants::getInstrCost();
+  return Metrics.NumInsts;
 }
----------------
ChuanqiXu wrote:
> Is this change necessary? Or what is the intention? It looks not good to me at the first sight.
Quoting a comment I left on the parent revision (D150375) in response to @mingmingl :
> The Specialization cost estimation is wrong and I am planning to fix that in later patches. Metrics.NumInsts already contains TargetTransformInfo::TCK_CodeSize.
See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/CodeMetrics.cpp#L180


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


================
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;
----------------
ChuanqiXu wrote:
> Oh, good idea. But I only understand what you're trying to do after reading the codes. A comment here will be helpful.
will do


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:640-642
+Cost FunctionSpecializer::estimateBranchInst(BranchInst *I, Value *V,
+                                             Constant *C,
+                                             ConstMap &KnownConstants) {
----------------
ChuanqiXu wrote:
> This looks similar with `estimateSwitchInst`. Can we try to merge them?
I can factor out common code to a static function perhaps.


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


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:718
+      auto Ops = ArrayRef(Operands.begin() + 1, Operands.end());
+      C = ConstantExpr::getGetElementPtr(I->getSourceElementType(), Ptr, Ops);
+    }
----------------
ChuanqiXu wrote:
> 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.
What alternative do we have?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150464