[llvm] [polly] [SimpleLoopUnswitch] Don't use BlockFrequencyInfo to skip cold loops (PR #159522)

[Teresa Johnson via llvm-commits]

teresajohnson wrote:

> > Agree, this change isn't going to be equivalent. I'm a little unclear of the problem being solved by this PR. What is the main motivation? There's a list of benefits but I'm not sure how important they are.
> 
> The main motivation is to remove the last use of BlockFrequencyInfo from within a loop pass, because I'm not sure if there's much value in relying on a potentially inaccurate result given that it's only preserved lossily.
> 
> I noticed this originally when some of the lossy results escaped into the loop vectorizer in #157888, causing BFI to return some bogus frequencies. Not off by one or two etc, but returning the frequencies for different blocks altogether since the CFG had been modified. I'm a bit worried that SimpleLoopUnswitch is also affected by this e.g. thinking a loop is cold when it's not.

Isn't this a larger problem though - downstream function passes such as BB layout will also be affected by incorrect BFI. I'm unclear why disabling use of BFI in this particular pass is the right approach to this issue.

> 
> There is a secondary motivation that avoiding code size increases should be handled by PGOForceFunctionAttrsPass. 

I'm not sure I agree here - why should code size increases from cold code only be gated at the function level? (I see you filed https://github.com/llvm/llvm-project/issues/159595, that seems like a good approach longer term.)

> I think it's somewhat coincidental that the original patch checked that the loops were cold instead of the function. As Nikita mentions, [fb45f3c](https://github.com/llvm/llvm-project/commit/fb45f3c9486f5d9e3003db95386432562b23577c) seems to suggest that the original author was fine with checking the function.

These messages reference SPEC, though, which isn't always the best reflector of real world codes.



https://github.com/llvm/llvm-project/pull/159522