[PATCH] D149918: [InstCombine] Add oneuse checks to shr + cmp constant folds.

Fri May 5 10:17:33 PDT 2023

aemerson added a comment.

In D149918#4322154 <https://reviews.llvm.org/D149918#4322154>, @goldstein.w.n wrote:

> In D149918#4321150 <https://reviews.llvm.org/D149918#4321150>, @aemerson wrote:
>
>> In D149918#4320934 <https://reviews.llvm.org/D149918#4320934>, @goldstein.w.n wrote:
>>
>>> Where are the codegen improvements coming from?
>>> Better `imm` values or something? Personally this feels a bit of a hack at the expense of potential ILP.
>>
>> I'll look into exactly what changed in the codegen tomorrow. Why does this feel like a hack as opposed to all the other hasOneUse() checks in the optimizers?
>
> The `hasOneUse` checks are there to prevent creating more instructions. Even with multi-use on `ashr` this combine
> doesn't create more instructions and does shrink the critical path so it passes muster for an InstCombine.

Ok, that's fair.

> If it is only darwin arm64 that cares about this change, why not handle it in the backend?

First of all, I didn't say that only Darwin arm64 cares about this, it's just the system that I happened to run the test suite build on. Secondly, these combines aren't easy to undo (at least one of them involves combing two constants into one). If anything, I think these combines make more sense to do at the codegen level than here but I could be wrong.

So looking at two cases, in oggenc there are quite a few occurrences of select patterns being broken by this combine (because the compare immediate is being munged into something not useful). These result in fewer simplifications which will be both larger and slower.

In MiBench/security-rijndael, we have as you say unfriendly immediate to compares being generated which causes us to have to materialize to registers. This is both a performance and code size hit.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149918