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

Fri Jul 28 10:43:19 PDT 2023

nikic added a comment.

I'm not a fan of the PGO conditional behavior here. I'd prefer to disable speculation unconditionally if that is feasible. This is basically what D153392 <https://reviews.llvm.org/D153392> did. While the specific test case there was addressed in a different way, if we take it in conjunction with this one, I think we have enough motivation to do the change.

Another piece of motivation would be that the EarlyFPM change would allow us to move LowerExpectIntrinsic to the end of EarlyFPM again, which would make expect intrinsics work in Rust again. The pass was moved earlier to make the first SimplifyCFG run take them into account, but that would no longer be a problem if we disable speculation.

And another piece of motivation would be that we could move SimplifyCFG after SROA, where it can be much more effective (but doing so with speculation breaks IPSCCP too much). I think doing that needs some further changes, but it's a move in the right direction.

So I think the EarlyFPM change at least seems like something we should pretty clearly do independently of PGO. The addPGOInstrPasses() change is of course PGO specific anyway. I think only the GlobalCleanupPM change may not be completely clear cut.

================
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:
> 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155997