[PATCH] D125248: [instcombine] Propagate freeze back to def

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D125248#3504103 <https://reviews.llvm.org/D125248#3504103>, @mtrofin wrote:

> In D125248#3503741 <https://reviews.llvm.org/D125248#3503741>, @nikic wrote:
>
>> I've put up https://reviews.llvm.org/D125321, which extends freezeDominatedUses() to move the freeze such that it dominates all uses (if possible).
>
> Yup, thanks!. I'm not super sure why we'd want to insert the new freeze first (potentially) in the middle of the original's uses, then hoist it to dominate all uses (rather than place it "at the right place" upfront), but likely I'm missing some design nuance of this pass. I'm not hung up on this - asking to learn.

Hoisting the freeze insertion point in pushFreezeToPreventPoisonFromPropagating() fixes the problem for freeze instructions introduced by that transform -- but it does not fix the problem for freeze instructions introduced by other (likely non-InstCombine) transforms. For that reason this should be handled by two separate folds that can apply independently of each other (though they would usually work in tandem).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125248