[PATCH] D98213: [InlineCost] Enable the cost benefit analysis on FDO

Fri Jan 20 14:39:06 PST 2023

davidxl added a comment.

In D98213#4070227 <https://reviews.llvm.org/D98213#4070227>, @aemerson wrote:

> I want to resurrect this change that enabled these cost-benefit heuristics originally developed in https://reviews.llvm.org/D92780
>
> There are a number of issues with the original change that I can see, and this has caused all manner of problems for us internally as we moved to the new pass manager (and therefore started using these heuristics).
>
> The main one being that the heuristics seem to completely override all sensible thresholds for code size. The feature caims to compare the performance gains by measuring the cycles saved from a PGO run, and comparing against some threshold. This doesn't make sense since the length of the runs for PGO are arbitrary, and you shouldn't be using them in absolute terms. The only way it makes sense is if you have a very specific workload and the PGO profile run is exactly the same as the real world runs. The original change gave absolutely no justification, formal or intuitive, for the use of the equation to determine profitability. Giving positive runtime results is no justification for completely overriding any reasonable thresholds and potentially impacting code size & compile time by several orders of magnitude as we've seen internally at Apple.
>
> Even worse: D92780 <https://reviews.llvm.org/D92780> was accepted into mainline LLVM with absolutely no tests, and then enabled for all users in this change. I didn't even see any discussion about tests, or explanation for why they were impractical. We've disabled these heuristics downstream but I absolutely believe that this must be reverted upstream too until it can be re-worked and re-reviewed in detail.

FYI: the cycle savings are normalized by dividing the function entry count, so it is not used in absolute sense.

I think it is worth adding some tests.

For the problems you have (I suppose it is performance related?) More details would be helpful (perhaps using a bug to document it).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98213