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

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopPassManager.h:408
 FunctionToLoopPassAdaptor<LoopPassT>
 createFunctionToLoopPassAdaptor(LoopPassT Pass, bool UseMemorySSA = false,
+                                bool UseBlockFrequencyInfo = false,
----------------
@asbirlea Assuming this change matches expectations of making BFI on present when LICM or loop passes contain LICM I'm looking at the users of this and they seem to fall into 2 categories:
1. Those that specify the optional flags
2. Those that only pass in the LoopPassT

I found as I was updating with a new flag that due to C++ behavior a place that wasn't updated to have all 3 optional parameters will place DebugLogging into UseBlockFrequencyInfo which is a nasty error. I think a way around it is to enforce overloaded functions with either 0 additional parameters or having every "optional" parameter specified so it'll be a build time error for the previous scenario rather than a runtime issue.


================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopPassManager.h:278
                                        AM.getResult<TargetIRAnalysis>(F),
+                                       &AM.getResult<BlockFrequencyAnalysis>(F),
                                        MSSA};
----------------
asbirlea wrote:
> This should not be unconditional. See MSSA approach.
Fixed it up to resemble MSSA


================
Comment at: llvm/test/Transforms/LoopRotate/pr35210.ll:51
+; MSSA-NEXT: Running analysis: BlockFrequencyAnalysis on f
+; MSSA-NEXT: Running analysis: BranchProbabilityAnalysis on f
 ; MSSA-NEXT: Running analysis: InnerAnalysisManagerProxy{{.*}} on f
----------------
asbirlea wrote:
> e.g. there's no use of creating these for LoopRotate.
Changed up so LoopRotate no longer has a dependency on BFI


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

https://reviews.llvm.org/D86156