[PATCH] D150375: [FuncSpec] Replace LoopInfo with BlockFrequencyInfo.

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:600-606
+  uint64_t Weight = BFI.getBlockFreq(I->getParent()).getFrequency() /
+                    BFI.getEntryFreq();
+  if (!Weight)
+    return 0;
 
-  // Increase the cost if it is inside the loop.
-  unsigned LoopDepth = LI.getLoopDepth(I->getParent());
-  Bonus *= std::pow((double)AvgLoopIters, LoopDepth);
+  Cost Bonus = Weight *
+      TTI.getInstructionCost(U, TargetTransformInfo::TCK_SizeAndLatency);
----------------
mingmingl wrote:
> (Hi! I notice this and was trying to get some generated function specialized so would like to share some thoughts :)  )
> 
> Function spec cost is calculated as ` Metrics.NumInsts * InlineConstants::getInstrCost()` (i.e., without TTI per instruction cost).
> I wonder if it would make more sense to make per-instruction cost calculation consistent.
> 
> Also it seems `Weight` could give a big bonus for functions in a hot inner-loop with PGO, wonder how it affects code size in PGO case.
The Specialization cost estimation is wrong and I am planning to fix that in later patches. `Metrics.NumInsts` already contains TargetTransformInfo::TCK_CodeSize. Regarding the weight, right now it only favors basic blocks with higher frequency than the entry (in other words, loops). In future patches I want to tweak the weight calculation as <SomeConstant> x <BlockFreq> / <EntrFreq> in order to allow conditional code to be accounted for the Bonus calculation. I haven't measured PGO configurations thoroughly, but I am planning radical changes to the cost model in the near future as enabling specializations of literal constants is becoming necessity and right now is too heavy weight.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150375