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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added a comment.

In D125248#3501446 <https://reviews.llvm.org/D125248#3501446>, @nikic wrote:

> I don't think this is the right place to do this. You should either:
>
> 1. Remove the one-use restriction on this fold, and replace other uses with the frozen variant as well. This is on the premise that pushing freeze higher is more worthwhile than having some un-frozen uses.
>
> 2. Adjust the freezeDominatedUses() fold to freeze not only dominated uses. Basically canonicalize %x to freeze %x. This is on the premise that allowing more exploitation of value identity is more worthwhile than having un-frozen uses.

I think in either case the `freeze %s` def needs to dominate its uses.

> I'm not sure which one of these is the right approach, and I didn't really understand what optimization is supposed to be achieved in your example -- you might want to construct it in a way that something actually folds away as a result.

Sorry, the full picture is more involved, and I simplified it quite a bit, on the assumption that moving the freeze to the def is what we'd want. 
In this example, we would want the uses of `%idx1` and `%idx2`, which aren't shown here, be optimized by simplifycfg. I can rework the example, but wanted to first check if there are any issues with moving the freeze up to dominate all the uses of the value it freezes (basically I couldn't figure out why the original change chose to move the freeze one up rather than all the way to the original def. Maybe @hyeongyukim remembers?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125248