[PATCH] D48662: [InstCombine] reverse canonicalization of binops to allow more shuffle folding

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D48662#1146670, @spatel wrote:

> In https://reviews.llvm.org/D48662#1146575, @lebedev.ri wrote:
>
> > I'm guessing this superseded https://reviews.llvm.org/D48485, which should be closed?
> >  Also, is https://reviews.llvm.org/D48678 supposed to go on top of this, or trunk?
>
>
> Each patch can be applied to trunk as-is. I posted this one to show one way that https://reviews.llvm.org/D48485 could be extended, so yes this one would obsolete https://reviews.llvm.org/D48485 if we want to make a larger change all at once. But my preference is to make the smaller change first to reduce risk. Ie, I think https://reviews.llvm.org/D48485 should be reviewed/committed, then we can make some NFC changes in preparation for this patch (and I need to add more tests). If we take that route, this patch will shrink to just adding one more 'case' to the switch.


Then i would suggest to do so now:

1. Update https://reviews.llvm.org/D48485 with this patch minus the `Instruction::Or` handling.
2. Update this https://reviews.llvm.org/D48662 - on top of https://reviews.llvm.org/D48485 (so the diff is relative), add `Instruction::Or` handling.

Will simplify reviewing, and signal the review order :)

> https://reviews.llvm.org/D48678 is also against trunk, so it can be reviewed as a stand-alone patch. I wanted to demonstrate that the 2-variable case is a small add-on either way. Given that, maybe it's easier to start there because that part won't have any follow-ups AFAIK.

Yeah, that one looks really simple. I'll try to review that one..


https://reviews.llvm.org/D48662