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

Hi, sorry for delay.

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

> Do these show up in practice?

No, the motivation was from example `test1_undef` of D84818 <https://reviews.llvm.org/D84818>: if A.fr (which is optimized to freeze(undef)) was combined with or/and & used by a conditional branch, opt was still generating a sub-optimal code.
I thought it must appear at least once in real-world code, but my experiments say that it did not fire when compiling SPEC/testsuite with -O3 + LTO, interestingly. :/
However, wouldn't these folds be pretty low-cost? I think this can happen in a larger codebase as well.



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84948