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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

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

> 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.

I think the second option really makes sense, I'll update this 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()));
+
----------------
nikic wrote:
> 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 :)
In this case, it is fine because seeing different values of an undef is allowed to the users of users transitively.
Since undef represents a set of any value conceptually, `and undef, x` is a set of value that is a subset of bits of x, and each use can pick a value which one to see.
This allows, e.g. folding `y = add undef, 1; use(y); use(y)` into `use(undef); use(undef)`.
Anyway, it is true that the behavior of undef is mysterious and hard to reason about..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84948