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

Tue Aug 29 12:13:08 PDT 2023

tejohnson added a comment.

In D155997#4570562 <https://reviews.llvm.org/D155997#4570562>, @aeubanks wrote:

> can we try not gating this on PGO as suggested? minimizing differences between pipelines is nice, and as mentioned it'll help with other cases

Sorry for the delay, was OOO for a bit and this ended up being a bit more complicated. Since I added a change to guard branch folding with the simplifyBlocks flag in D156194 <https://reviews.llvm.org/D156194>, there were additional tests that had differences that I have been trying to analyze and mitigate, with partial but not complete success (i.e. additional to what was seen when @aeubanks  tried this in D153392 <https://reviews.llvm.org/D153392>):

1. llvm/test/CodeGen/Hexagon/loop-idiom/pmpy-mod.ll

The Hexagon loop idiom recognition pass is a little rigid in the detected code sequences, which were different because once we did the branch folding in a later SimplifyCFG invocation, followed by InstCombine, there was no EarlyCSE before the hexagon passes to do the cleanup required to get the old sequence. I can fix this by adding an invocation of EarlyCSE after those passes in buildFunctionSimplificationPipeline. There were no test changes from that fix, other than some tests that check the order of passes in each pipeline.

2. llvm/test/Transforms/PhaseOrdering/X86/vector-reductions-logical.ll

Not all function in this test are affected, but several are affected due to the phase ordering, specifically we stopped doing some of the branch folding completely which blocked some of the vectorization handling. In one of the functions I examined I found this was due to the fact that an EarlyCSE pass before we reran SimplifyCFG with speculation enabled did some commoning that blocked branch folding due to a limitation in the handling there. Specifically, the one added in https://reviews.llvm.org/rG909cba969981032c5740774ca84a34b7f76b909b. Undoing that does make the branch folding kick in more in this test, but isn't correct in general (it sounds like that patch was submitted to address a problem that was difficult to solve more optimally). In fact, that change had some similar affects on this test to my change, but there have been quite a few changes impacting this test over the past few years.

3. clang/test/Headers/__clang_hip_math.hip

Some fairly superficial diffs that don't appear important to the test.

Thoughts on how should I proceed? I can send the simple change I did for 1) above separately, since it doesn't seem to do any harm and presumably EarlyCSE is cheap. For 2) however, I'm not sure how much effort I should invest to get the test to behave exactly the same as before. There is clearly a limitation with SimplifyCFG that has been looked at before but isn't easy to solve.

================
Comment at: llvm/test/Transforms/PhaseOrdering/simplifycfg-speculate-blocks.ll:1
+;; Check that SimplifyCFG does not attempt speculation until after PGO is
+;; annotated in the IR, and then does not perform it when unprofitable.
----------------
aeubanks wrote:
> nikic wrote:
> > aeubanks wrote:
> > > hmm typically these phase ordering tests use `llvm/utils/update_test_checks.py`, but that doesn't support the llvm-profdata RUN line. I think a non-update_test_checks test is probably fine for this, @nikic does that make sense?
> > > 
> > > 
> > I thought UTC supports unrecognized commands (by just executing them)... If not, a non-UTC test is fine.
> `update_test_checks.py` doesn't handle `%t` (maybe that's an improvement we could make to the script)
I'm not sure what the alternative is here to using %t - do we need make the test support update_test_checks.py?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997