[PATCH] D58633: [InstCombine] remove one-use restriction for icmp+add constant fold

Mon Dec 2 07:14:00 PST 2019

lebedev.ri added a comment.

In D58633#1468821 <https://reviews.llvm.org/D58633#1468821>, @spatel wrote:

> In D58633#1465428 <https://reviews.llvm.org/D58633#1465428>, @nikic wrote:
>
> > There's a similar one use check for the non-equality case in https://github.com/llvm-mirror/llvm/blob/fb1e04ff3c51a617c4944b2e05e8aa1b8c543e22/lib/Transforms/InstCombine/InstCombineCompares.cpp#L2389. This prevents the trivially true condition in https://rust.godbolt.org/z/qg0RjC from folding. That check was added (or rather moved) in rL341831 <https://reviews.llvm.org/rL341831>.
> >
> > Just wanted to add this as a datapoint for a case where dropping the restriction would be nice.
>

That should be resolved now, reverted in rG0f22e783a038 <https://reviews.llvm.org/rG0f22e783a038b6983f0fe161eef6cf2add3a4156>.

> It's not explicitly stated in the commit message, but the test added with rL341831 <https://reviews.llvm.org/rL341831> suggests we are missing some kind of overflow check optimization (cc @t.p.northover)? Or maybe the motivation was similar to the cases we have here - covering up for failures in LSR?
> 
> Either way, we have a real motivating example for correcting instcombine now. If there's not enough will to fix the underlying problems, we can probably justify adding an over-specific match to instsimplify to zap the compare in the rust example.

I've talked to @t.p.northover, and the original motivation is unknown/unreconstructible now.
That test is misleading - it wouldn't be transformed before rL341831 <https://reviews.llvm.org/rL341831> anyway - there are no no-wrap flags on the `add` there.

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

https://reviews.llvm.org/D58633