[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;
 }
----------------
labrinea wrote:
> 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
Oh, sorry for disturbing.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:686
 
+Cost FunctionSpecializer::getUserBonus(Instruction *User, Value *Use,
+                                       Constant *C, ConstMap &KnownConstants) {
----------------
labrinea wrote:
> ChuanqiXu wrote:
> > Maybe we can make it look prettier by using `InstVisitor` for such code patterns.
> Can you give an example?
There are multiple examples: InstructionAllowed and CallAnalyzer and so on. Maybe you would feel the code become longer after we adopt that. But I think it will be easier to read and understand. This is a suggestion and not a requirement.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:718
+      auto Ops = ArrayRef(Operands.begin() + 1, Operands.end());
+      C = ConstantExpr::getGetElementPtr(I->getSourceElementType(), Ptr, Ops);
+    }
----------------
labrinea wrote:
> 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?
I don't know if we have an off-the-shelf solution for this. But I'm sure that the resulting 'C' here shouldn't be ConstantExpr. If there is not an off-the-shelf solution, maybe we can add one for it. Or we can leave the case for later patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150464