[llvm] [FuncSpec] Enable SpecializeLiteralConstant by default (PR #113442)
Alexandros Lamprineas via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 24 04:32:30 PDT 2024
================
@@ -682,10 +679,9 @@ bool FunctionSpecializer::run() {
(RequireMinSize && Metrics.NumInsts < MinFunctionSize))
continue;
- // TODO: For now only consider recursive functions when running multiple
- // times. This should change if specialization on literal constants gets
- // enabled.
- if (!Inserted && !Metrics.isRecursive && !SpecializeLiteralConstant)
+ // When specialization on literal constants is disabled, only consider
+ // recursive functions when running multiple times.
+ if (!SpecializeLiteralConstant && !Inserted && !Metrics.isRecursive)
----------------
labrinea wrote:
You changed the comment but not the condition (other than reordering it). Perhaps the TODO wasn't clear when I wrote it. Sorry. I meant to say that when we allow specializations on literal constants, a specialization may return a constant. Therefore the SCCP Solver needs to invalidate the LatticeValue for the users of that returned value https://github.com/llvm/llvm-project/blob/c07abf727220e3bd2c78b129a683ad79f735e43c/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp#L804 and to recalculate it https://github.com/llvm/llvm-project/blob/c07abf727220e3bd2c78b129a683ad79f735e43c/llvm/lib/Transforms/IPO/FunctionSpecialization.cpp#L810
So we used to only consider re-evaluating recursive functions on subsequent iterations of the Specializer, but now we should consider everything. I think when I wrote the TODO I was thinking that we may want to remove this check completely, but it make sense to keep it in case SpecializeLiteralConstant=false to avoid repeating the same computations since no lattice value could have changed from the previous iteration. Can you reflect what I wrote here in the comment? Reordering the conditions seem fine to me.
https://github.com/llvm/llvm-project/pull/113442
More information about the llvm-commits
mailing list