[PATCH] D65060: [LICM] Make Loop ICM profile aware

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

This approach is broken for another reason, which also motivated the LoopSink approach David mentioned.

BlockFrequencyInfo isn't preserved by loop passes. This is deeply problematic. In the legacy pass manager, this almost works, but can result in wildly inconsistent behavior when half way through a loop pipeline BFI gets invalidated and/or where it can partition the loop pass pipeline in weird ways breaking intended iteration order of the loop pass pipeline. With the new pass manager, I suspect this produces non-deterministic behavior based on what order we visit things causing BFI to be present or not when we happen to reach the loop pass manager, and happening to change the behavior. Even worse, if BFI *happens* to be available at the start of the loop pass pipeline in the new pass manager, it will continue to be used even after passes restructure the basic blocks invalidating it. This can lead to arbitrarily bad behavior.

To do something like this, we really need to teach all the loop passes in the same loop pass pipeline as LICM that is set up this way to update and preserve BFI. This will fix the weird behavior w/ the legacy PM and it will allow you to add the analysis as one of the guaranteed loop analyses like MemorySSA and others.

This is exactly what we had to do for MemorySSA and we'll have to do the same thing here.

To be clear, we've already seen this cause stage2/3 differences and other problems when PGO is enabled (crashes, miscompiles).

Hope my explanation helps, but we'll need to revert this ASAP.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65060