[PATCH] D84948: [InstCombine] Fold select/and/or with freeze(undef) that is used in the op only

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D84948#2197585 <https://reviews.llvm.org/D84948#2197585>, @nikic wrote:

> Just as another possibility, we could simply always fold `freeze undef` to `zeroinitializer` in InstCombine (regardless of number of uses). This is not always the optimal fold, but usually a good one. At least at this point, I don't think `freeze undef` is expected to be common and it's more important to get rid of the freeze than find the best replacement.

To expand on this, if we want to handle multi-use freeze undef in the future, there's generally two ways that can work:

1. Individual folds for certain operators (as done here), but replacing the freeze itself with an advantageous value rather than just the operator.
2. Walking the uses of the freeze to find the best replacement value and replace all uses. (E.g. by counting for how many uses a certain value is favorable.)

The current patch would be the starting point for 1, my suggestion would be the starting point for 2.

I should say though that I'm also ok with your current patch.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1803
+      match(Op1, m_OneUse(m_Freeze(m_Undef()))))
+    return replaceInstUsesWith(I, Constant::getNullValue(Op0->getType()));
+
----------------
aqjune wrote:
> nikic wrote:
> > So, these changes duplicate some undef folds from InstSimplify (here: https://github.com/llvm/llvm-project/blob/9b04fec0021112d913058e81bf864e4f33c33dcc/llvm/lib/Analysis/InstructionSimplify.cpp#L2004-L2006) into InstCombine.
> > 
> > I think it would be legal to instead make the fold in InstSimplify also work for one-use freeze of undef. Per the rules, the InstSimplify result needs to be used for a replacement, which will also remove the freeze. Of course, this will only really be safe in conjunction with D84792 for consumers that don't perform replacements.
> > 
> > Though maybe, if we consider the long-term direction (undef replaced by freeze poison), having it in InstCombine and dropping the InstSimplify folds may make more sense -- because we could also fold the multi-use case here, if we replace the whole freeze instruction.
> I think it depends on whether it is valid to replace a portion of uses with InstSimplify-ed value.
> For example, even freeze has only one use, if and/or has multiple uses, this can still be problematic:
> ```
> y = and (freeze undef), x
> use(y)
> use(y)
> ```
> If (for some reason) the first use(y) is simplified to 0 but not the second use(y), the two uses will see different values, which is not valid.
> 
> If InstCombine isn't enough, we can properly fix each optimization to deal with it, IMO (e.g. JumpThreading). This seems to be a safer way to fix, albeit it may need more engineering.
I don't think the InstSimplify contract allows replacing only some uses. If you replace your example with

```
y = and undef, x
use(y)
use(y)
```
and you replace only the first use with 0, then the second one could later get folded to x still, observing two different values of undef. I don't think the freeze makes a difference here.

Of course, not doing it in InstSimplify is still safer :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84948