[PATCH] D84250: [InstSimplify] ExpandBinOp should fold an expression only when it's safe

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune created this revision.
aqjune added reviewers: craig.topper, lebedev.ri, spatel, nlopes.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

ExpandBinOp folds `(A op' B) op C` by transforming it into
`(A op C) op' (B op C)` first, simplifying the two subexpressions
(say the result as `L` and `R`), and folding `L op' R` if possible.

However, if `C` contains undef, this results in making undef have two different
values whereas the original expression may not;
as a simplest case, transforming `undef * (1 + 1)` into `undef + undef` isn't
safe, because the former one is an even whereas the latter one is any number.

This patch makes ExpandBinOp fold only when it's safe.

1. If either `L` or `R` is special in that `L op' ?` or `? op' R` always folds

into a constant, it is safe to fold the expression to the constant.

2. If `C` is never undef or poison, it is safe.

After this patch, transformation https://reviews.llvm.org/D83360#2146539 does not happen
after D83360 <https://reviews.llvm.org/D83360> is enabled.

At https://reviews.llvm.org/D83360#2146539 , I proposed making InstSimplify
remember to which value undefs are folded & use the cached value if the undef
is met again.
My plan was to implement this solution by adding a static `ValueOrUse` class
(so we can distinguish undefs by Use) and letting InstSimplify's functions
use it instead of `Value`. But, it had several problems.

1. If InstSimplify fails to simplify values, the record of folded undefs should

be rewinded at function returns. Doing this all over InstSimplify is
error-prone and easy to miss.

2. Sometimes the value that undef is folded isn't just a constant. For example,

if `undef + 1 <= X` is folded into `true`, the undef is folded into either -1
or a random number between [0, X-1]. Recording this isn't trivial.

3. Constant folding can fold undefs too. This causes changes in the relevant

libraries.

4. Finally, if InstSimplify becomes smart enough, this still can cause miscompilation.

For example, if InstSimplify skims through instructions in the same basic block
and find equivalent expressions, a miscompilation can happen
without explicit folding of undefs.

Another approach that I tried was to simply
stopping undef folds when called from ExpandBinOp recursively.
However, this also required a nontrivial change because two
UndefValue instances compare equal, leading to undesirable foldings like
`undef - undef --> 0`.
Also, it still does not resolve the possible miscompilation problem.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D84250

Files:
  llvm/lib/Analysis/InstructionSimplify.cpp
  llvm/test/Transforms/InstCombine/and-or-icmps.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D84250.279568.patch
Type: text/x-patch
Size: 8202 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200721/fbc4d378/attachment.bin>