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

Thu Feb 28 14:13:18 PST 2019

spatel planned changes to this revision.
spatel added a comment.

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

> Yep. I think that makes sense to me. Would be good to remove the hack. And LSR should handle most of these cases. I think there's times when it doesn't do what it should, though.
>
> This is one example, out of libjpeg I think, but I got it from csibe. It's was just the first I looked at that was larger than before:
>  https://reviews.llvm.org/P8129
>  Compiled with something like "opt -instcombine -o - -S jdmarker.ll | llc - -o -", vs without the instcombine. Codesize is a little bigger than it was, I think maybe something about the loops (not in simple form?) is making the SCEV's not useful, so LSR isn't transforming these back.
>
> I don't know if this is representative of the other changes. Over all the benchmarks I ran, this patch seemed to be (on average, very slightly) worse overall than before, both for perf and codesize. Not by a long way, and there are certainly some improvements in there.

Thanks for checking that. I see x86 getting bigger on that example too.
Given that this is just an academic change for me currently (we ended up needing to match both patterns for unsigned add overflow in the patches that led me here), I think we better try to make LSR stronger before pushing this one. I'm guessing this won't be at the top of my queue for a while, but I'll mark it as 'Plan changes' for now.

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

https://reviews.llvm.org/D58633