[PATCH] D139275: [SimplifyCFG] `FoldBranchToCommonDest()`: deal with mismatched IV's in PHI's in common successor block

Tue Dec 20 15:00:27 PST 2022

lebedev.ri added a comment.

In D139275#4009140 <https://reviews.llvm.org/D139275#4009140>, @aeubanks wrote:

> just wanted to note that this did increase binary size in some places (https://bugs.chromium.org/p/chromium/issues/detail?id=1401820),

It's almost assuredly second-order effects.

> even though the description claims it will basically never fire. (I'm not blocking anything on that, just a note)

That description is based off of it's current cost model, which is extremely, extremely, conservative.

> separately, IMO this patch is non-trivial enough to deserve an lgtm before landing

There are several things to consider in this cases:

- Trivial-ness is different for everyone. I think, this is reasonably straight-forward.
- Review is useful to catch issues, BUT it is even more important to learn from previous issues to not repeat them, both to reduce review cycles, especially so when no adequate review can be requested/acquired.
- We seem to not have many active contributors in the areas i touch, so if i always just wait for review, i will be waiting a long while. And at the same time, not learn from mistakes, and not be able to review other's code.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139275