[PATCH] D154579: [InstCombine] Only perform one iteration

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D154579#4477044 <https://reviews.llvm.org/D154579#4477044>, @RKSimon wrote:

> This seems worthwhile in pursuing, but I don't know very much about how IC worklists are managed/sorted - your summary implies there are a number of workarounds in there, would these benefit from being cleaned up before/after this change?

Normally, InstCombine worklist management is handled implicitly, using a combination of IRBuilder callbacks and standard helpers like replaceInstUsesWith(). Things work automatically as long as folds just replace one sequence of instructions with another. However, for folds that do non-local changes (e.g. looping over users and doing extra replacements there), it may be necessary to perform manual worklist management. I've been working on adding that manual worklist management in all the places that were missing it over the last few weeks, and I'm not aware of any remaining issues. (The most common case is folds leaving behind dead instructions without queuing them for DCE.)

> Regarding your verify-fixpoint proposal - would it mean we don't have much "no-verify-fixpoint" test coverage apart from phase-ordering or similar tests?

Right. Ideally we would always verify the fixpoint for tests (so that an explicitly opt-out is required for cases that don't reach the fixpoint), and not verify it outside tests. Verifying it for `-passes=instcombine` but not `-passes='default<O3>'` would be the heuristic for that.


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

https://reviews.llvm.org/D154579