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

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D84250#2187617 <https://reviews.llvm.org/D84250#2187617>, @nikic wrote:

> Assuming D84792 <https://reviews.llvm.org/D84792> lands, would performing the two SimplifyBinOp queries for L and R with CanUseUndef=false solve this problem as well? I would prefer that if it works.

I think there is a trade-off between full correctness and performance. If InstSimplify can be arbitrarily smart in the future, simply blocking the transformation as shown in this patch seems to be the right way IMO.
For example, InstSimplify in the future can iterate over the BB and find equivalent instructions (as GVN does): in this case, a miscompilation can still happen, because distributivity can lead to replacing `2 * x` with `x + x`.
Also, another concern that I had with bringing the can-undef-fold flag was that the change was global: whenever we add a new optimization to InstSimplify, it should consider whether CanUseUndef was correctly considered (maybe ConstantFolding, too?).

However, InstSimplify may not need to be very smart, and the cost of maintenance may not be very big in practice as well.
I am willing to follow the result of discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84250