tejohnson added a comment. In D155997#4543156 <https://reviews.llvm.org/D155997#4543156>, @nikic wrote:
> 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. It looks like D153392 <https://reviews.llvm.org/D153392> does roughly the same disabling in practice, but you are right it doesn't gate on whether we will eventually feed back profile info. For the issue I was trying to solve, it was because of the PGO based profitability checks in SimplifyCFG speculation, but if it makes sense in other cases to disable it until later, then that works for me. BTW doesn't look like that patch disables speculation in addPGOInstrPasses, which we need for the issue I'm trying to solve. > 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. For the case I was looking at, with instrumentation PGO, any of the pre-feedback SimplifyCFG speculation passes would do the wrong thing without that PGO data. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155997/new/ https://reviews.llvm.org/D155997 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
