[PATCH] D86156: [BFI] Preserve BFI information through loop passes via VH callbacks inside LoopStandardAnalysisResults

[Di Mo via Phabricator via cfe-commits]

modimo added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:522
   FPM.addPass(createFunctionToLoopPassAdaptor(
-      std::move(LPM2), /*UseMemorySSA=*/false, DebugLogging));
+      std::move(LPM2), /*UseMemorySSA=*/false, /*UseBlockFrequencyInfo=*/true,
+      DebugLogging));
----------------
asbirlea wrote:
> It doesn't look like `UseBlockFrequencyInfo` is used for LPM2 here and below. Would it make sense to set it to `false` at this point?
> 
Good catch, changed to false for LPM2.


================
Comment at: llvm/test/Other/pass-pipelines.ll:57
 ; CHECK-O2: Loop Pass Manager
-; CHECK-O2-NOT: Manager
+; Requiring block frequency for LICM will place ICM and rotation under separate Loop Pass Manager
 ; FIXME: We shouldn't be pulling out to simplify-cfg and instcombine and
----------------
asbirlea wrote:
> Add `; CHECK-O2: Loop Pass Manager` along with this comment.
> 
> 
> Please consider if splitting the loop pass pipeline has any effects on optimizations. This is for the legacyPM only, so those who switched to the newPM will not be affected.
> The solution may be to mark the analyses preserved in loop unswitch.
To check my understanding here, with the split loop pass pipeline the phases look like the following:

```
Lazy Branch Probability Analysis
Lazy Block Frequency Analysis
Loop Pass Manager
  Loop Invariant Code Motion
Loop Pass Manager
  Unswitch loops
```
Walking through the code of Loop Pass Manager by itself it doesn't re-calculate or produce additional analysis. Thus the difference appears to arise as follows:
1. combined loop pass: loop opts are run one after the other per loop, so if you have loops L1 and L2 the order would be L1(LICM, Unswitch) -> L2 (LICM, Unswitch)
2. split loop pass: in this case it would be L1(LICM)->L2(LICM) into L1(Unswitch)->L2(Unswitch)

My qualitative assessment is that the impact here is quite minimal. The main scenario I can think of where differences could occur is having all LICM completed early can change the costing in unswitching for nested loops. 

Unswitching only occurs once in the legacyPM and always after LICM so it seems fairly tame to build in this cross-pass dependence by marking lazy BFI/BPI preserved. I'd like to know if this level of cross-pass dependence is potentially an issue and if there's precedence for doing so if you have that context.

I've made the changes in the latest commit so that they exist in the same pass pipeline again by marking lazy BFI/FPI as preserved in unswitching. The value is definitely there to prevent unwanted side-effects.


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

https://reviews.llvm.org/D86156