[PATCH] D129599: Apply PGO on SimpleLoopUnswitch

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

the title should be something like `[SimpleLoopUnswitch] Skip non-trivial unswitching of cold loops`

otherwise this looks like the right approach



================
Comment at: llvm/lib/Passes/PassBuilder.cpp:1398
       bool UseBFI = llvm::any_of(
-          InnerPipeline, [](auto Pipeline) { return Pipeline.Name == "licm"; });
+          InnerPipeline, [](auto Pipeline) { return Pipeline.Name.contains("licm") || Pipeline.Name.contains("simple-loop-unswitch"); });
       bool UseBPI = llvm::any_of(InnerPipeline, [](auto Pipeline) {
----------------
run `git clang-format`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3121
+    AM.getResult<FunctionAnalysisManagerLoopProxy>(L, AR)
+       .getCachedResult<ModuleAnalysisManagerFunctionProxy>(F)
+       ->getCachedResult<ProfileSummaryAnalysis>(*F.getParent());
----------------
as we found offline, this may return `nullptr` in the case of `opt -aa-pipeline=`, we should bail out in that case

I'd add a `RUN: opt -passes=simple-loop-unswitch -aa-pipeline=` to the test to make sure it doesn't crash (no need to check IR)


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3237
   auto &TTI = getAnalysis<TargetTransformInfoWrapperPass>().getTTI(F);
+  auto BFI = nullptr;
+  auto PSI = nullptr;
----------------
just directly pass `nullptr` below


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp:3287
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
+INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(DominatorTreeWrapperPass)
----------------
these are specific to the legacy pass and don't need to be here since we're not actulaly using BFI in the legacy pass


================
Comment at: llvm/test/Transforms/SimpleLoopUnswitch/PGO-nontrivial-unswitch.ll:2
+
+; RUN: opt < %s -passes='require<profile-summary>,function(loop-mssa(simple-loop-unswitch<nontrivial>)),print<loops>' \
+; RUN:     --disable-output 2>&1 | sort -b -k 1 | FileCheck %s
----------------
I'd just use `llvm/utils/update_test_checks.py` rather than manually checking loops
once the test looks good it should be precommitted so we can see the delta this patch gives


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129599