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

Thu Feb 28 06:26:55 PST 2019

spatel added a comment.

In D58633#1413514 <https://reviews.llvm.org/D58633#1413514>, @dmgreen wrote:

> It looks like this was added on purpose way back in https://reviews.llvm.org/rL16473. That was so long ago that everything will have changed since then, but have you run any benchmarks for this?

Thanks for digging up the original commit! So it was a purposeful hack. :)
The problem with that hack of course is that crippling a canonicalization is not the same as adding the inverse canonicalization. Also, we are already doing this same transform for related patterns - there was just this odd-shaped hole carved out by this use limitation for some subset of constant values.

I did some more experiments, and my guess about LSR (-loop-reduce) was correct. It cleans up the IR for the motivating example in either incoming form (before/after this patch) to exactly the same output form for a given target. I checked x86, PPC, AArch64, and armv7 triples. I only have x86 hardware to run benchmarks, but I don't see anything above noise with this patch. Note that the LSR behavior holds for both -O1 and -O2 (potentially drastically different IR due to loop vectorization).

> I agree that LSR should //probably// be turning this back into a cmp based on the inc in any loop backedges. But I'm not sure it will do that equally well for all backends. And changing the canonical form of a loop backedge is quite a big change :)
>  Oh, and I presume most loop transforms will use SCEV's, which won't have changed? So we don't need to update the tests for them all, to make sure they are still doing what they should?

I'm not too familiar with SCEV. I see that LSR uses them, and that output matches expectations. Is there some other experiment I can try to confirm?

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

https://reviews.llvm.org/D58633