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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:686
 
+Cost FunctionSpecializer::getUserBonus(Instruction *User, Value *Use,
+                                       Constant *C, ConstMap &KnownConstants) {
----------------
ChuanqiXu wrote:
> 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.
Ah, you meant the instruction visitor like the one used by the SCCPSolver. I was experimenting with this idea yesterday. One problem I see is that the visit methods take only one argument (the visited instruction) so we would have to cache the extra parameters inside the visitor class. I'll investigate more. 


================
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:
> 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.
I think we can use the data-layout agnostic constant folding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150464