[PATCH] D45842: [Reassociate] swap binop operands to increase factoring potential

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D45842#1090363, @spatel wrote:

> Patch updated:
>  I think what made this patch confusing and overly complex is trying to recycle existing instructions (swapping operands rather than just creating new instructions).
>
> As I mentioned in an earlier comment, I don't see a good reason to do that, so let's do the easy thing: use IRBuilder to create new instructions. This has 3 benefits and shrinks the patch:
>
> 1. It simplifies the code needed for commutative canonicalization (looks more like the original instcombine patch now).
> 2. It means we don't need to manually update debug info (IRBuilder does the right thing automatically).
> 3. It makes the IR flag clearing/propagation cleaner.


Nice!
Looks much better, much easier to follow the logic / read the code.

I think this is ok, but maybe wait for a second opinion, i don't trust myself too much..


https://reviews.llvm.org/D45842