[PATCH] D46336: [InstCombine] Apply binary operator simplifications to associative/commutative cases.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D46336#1087159, @yamauchi wrote:

> Re: bloat, is the consensus that we just don't add to instcombine any more?
>
> This thread http://lists.llvm.org/pipermail/llvm-dev/2017-July/115398.html seemed to have concluded that it’s still acceptable to add to instcombine?


Thanks for finding that thread...somehow I find it difficult to search llvm-dev history...
Yes, I think we can still add to instcombine, but I am skeptical of large changes like this that alter the run loop. It doesn't fit in the spirit of small, constant-time combining.

> If instcombine is going to be split into multiple passes, assuming we can't call instcombine simplification routines from another pass (unlike instsimplify), could we end up with a situation where we would need to run another pass and instcombine in turn multiple times until a fix point? I'm not sure if any of the extra things that are currently (or proposed to be) part of instcombine are in fact like this and is in instcombine because of this. For example, factorization may open up new other instcombine opportunities, which in turn opens up more factorization opportunities, etc. Anyhow, it seems non-trivial.

I agree that it's not always clear (and note that I posted an alternative version of https://reviews.llvm.org/D45842 in PR37098 that would do more reassociation in instcombine...it would be easier!).

I share your goal of getting these folds to occur, but I'm not an authority on anything, so I think you should raise this discussion on llvm-dev to get a better answer.


Repository:
  rL LLVM

https://reviews.llvm.org/D46336