[PATCH] D84609: [MemDepAnalysis] Cut-off threshold reshuffling

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D84609#2180216 <https://reviews.llvm.org/D84609#2180216>, @fhahn wrote:

> In D84609#2180117 <https://reviews.llvm.org/D84609#2180117>, @asbirlea wrote:
>
>> In D84609#2179889 <https://reviews.llvm.org/D84609#2179889>, @lebedev.ri wrote:
>>
>>> In D84609#2179323 <https://reviews.llvm.org/D84609#2179323>, @asbirlea wrote:
>>>
>>>> Some general comments:
>>>>
>>>> - The MemDepAnalysis has been known to be problematic for compile-time, so reducing the 100k implicit threshold seems reasonable.
>>>> - It's natural the compiler tracker will be happy, but can we consider runtime implications due to potential missed optimizations?
>>>> - Could we evaluate which optimizations are missed, in which passes using MemDepAnalysis along with their run-time impact? (in particular for the benchmarks where we see the large compile-time benefits)
>>>>
>>>> AFAIK, the main passes using MemDepAnalysis are DSE, MemCpyOpt and GVN and there is active work in porting these to MemorySSA. The same analysis of compile-time vs run-time benefits is needed for that switch, so having data from reducing thresholds here will be very valuable for the short term (current patch) on deciding how much to reduce it to, and for the long-term switch to MemorySSA.
>>>
>>> Background: the motivation for this patch is the fact that D84108 <https://reviews.llvm.org/D84108> significantly
>>> regresses compile-time of lencod, especially `context_ini.c` (+79.97%).
>>> It has been traced to GVN spending most of the time in `GVN::processNonLocalLoad()`,
>>> in `MemoryDependenceResults::getNonLocalPointerDependency()`.
>>>
>>> Since a compile-, and run- time performance assessment will be needed both here,
>>> and in MemorySSA switch, would it be more productive to directly proceed to the latter?
>>
>> I think the two are orthogonal. The current GVN will need replacing with NewGVN which has been bitrotting for a few years now. It's a pass taking a different work-flow, so I don't know if there will be a call-path matching the `GVN::processNonLocalLoad()` call for example.
>> It will be very important for the subsequent switch to have the data point of "GVN performs this processing which has relevant run-time impact, regardless of the compile-time spent, hence NewGVN needs to match this" vs "GVN spends a lot of time compiling this without much or any run-time benefit, hence NewGVN need not match it".
>> I understand it's time-consuming to have this analysis now, but the problem is more complex than the lowering of a cap, and it seems important for future progress to distinguish between such cases, and document the results accordingly (in MemDepAnalysis, but more importantly in GVN).
>
> FWIW I think current GVN does too many things not directly related to value numbering, which makes the comparison with NewGVN a bit harder.  I think it might make sense to de-couple some of the memory optimizations that do not really interact with the value number from GVN.

I wasn't really asking about NewGVN story, i know it's stagnant somewhat.
I was only asking, would it be better to instead look into porting
`GVN::processNonLocalLoad()` to be MemorySSA-driven.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84609