[PATCH] D155997: [Phase Ordering] Don't speculate in SimplifyCFG before PGO annotation

Mon Jul 24 15:56:21 PDT 2023

aeubanks added inline comments.

================
Comment at: llvm/test/Transforms/SimplifyCFG/fold-branch-to-common-dest.ll:3
 ; RUN: opt < %s -S -passes=simplifycfg -simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | FileCheck %s
+; RUN: opt < %s -S -passes='simplifycfg<no-speculate-blocks>' -simplifycfg-require-and-preserve-domtree=1 -bonus-inst-threshold=10 | FileCheck %s --check-prefix=NOSPECULATE

----------------
tejohnson wrote:
> aeubanks wrote:
> > these checks won't work for a `update_test_checks.py` test. should add tests like the one added in https://reviews.llvm.org/D153391
> Can you clarify - are you saying remove the changes from this test and add a new test for this change like llvm/test/Transforms/SimplifyCFG/speculate-blocks.ll, or change the checks here?
revert the changes here (`update_test_checks.py` tests shouldn't have manually added CHECK lines) and add new function(s) to `llvm/test/Transforms/SimplifyCFG/speculate-blocks.ll` that show the difference between `speculate-blocks` and `no-speculate-blocks`

================
Comment at: llvm/test/Transforms/SimplifyCFG/pipeline-delay-speculation-pgo.ll:1
+;; Test that the pipelines delay simplify CFG speculation until after
+;; pgo annotation.
----------------
tejohnson wrote:
> aeubanks wrote:
> > do you have a phase ordering test instead?
> Can you clarify what you are referring to?
`llvm/test/Transforms/PhaseOrdering` has IR tests that get run through an entire pipeline (e.g. `thinlto-pre-link<O3>`) to verify some property of the entire pipeline. ideally you can get some somewhat-reduced IR that exhibits the problem you were seeing before the change and show that it improves with the pipeline change. you'll need a profile as part of that test I suppose

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997