[PATCH] D148210: [InstCombine] icmp(MulC * X * Y, C) --> icmp(X * Y, C)

[Jun Zhang via Phabricator via llvm-commits]

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

In D148210#4264845 <https://reviews.llvm.org/D148210#4264845>, @nikic wrote:

> In D148210#4264657 <https://reviews.llvm.org/D148210#4264657>, @junaire wrote:
>
>> In D148210#4264628 <https://reviews.llvm.org/D148210#4264628>, @nikic wrote:
>>
>>> As I understand it, this is a reassociation problem. Once you reassociate the multiply from `(X * C) * Y` to `(X * Y) * C` this will automatically fold via the existing logic in foldICmpMulConstant(). I don't really want this logic to be duplicated.
>>>
>>> It looks like `reassociate` currently produces this form with the constant on the inner multiply, but I'm not sure whether that's something that can be changed or not.
>>
>> Thanks, that makes sense and solve my confusion when I wrote this. Should I try to fix the missing optimization by looking into the reassociate issue or the patch doesn't make sense at all?
>
> I took a quick look at `reassociate`, and I don't think it's possible to change what it does. Reversing the order is not really compatible with the ranking it performs.
>
> In that case, I think we do want to handle this here, but we should try to extract the common code from foldICmpMulConstant(). I'm thinking that the helper function should accept the multiply constant, the icmp constant, the predicate and the OBO flags, and then return the new predicate and constant if possible. Then we'll call that function to check whether the reassociated form can fold and if so, we can actually materialize the new multiply and the new icmp.

Thanks, I'll try what you proposed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148210