Page MenuHomePhabricator

[InstCombine] fold shuffle-with-binop and common value

Authored by spatel on Jul 2 2018, 6:59 AM.



This is the last significant change suggested in PR37806:
...though there are several follow-ups suggested in the code comments in this patch to complete this transform.

It's possible that a binop feeding a select-shuffle has been eliminated by earlier transforms (or the code was just written like this in the 1st place), so we'll fail to match the patterns that have 2 binops from D48401, D48678, D48662, D48485.

In that case, we can try to materialize identity constants for the remaining binop to fill in the "ghost" lanes of the vector (where we just want to pass through the original values of the source operand).

I added comments to ConstantExpr::getBinOpIdentity() to show planned follow-ups. For now, we only handle the 5 commutative integer binops (add/mul/and/or/xor).

Diff Detail


Event Timeline

spatel created this revision.Jul 2 2018, 6:59 AM
spatel added a comment.Jul 2 2018, 7:08 AM

Note: this patch is against trunk. It should patch cleanly independently of D48662 (but there would be offsets if that goes in 1st).

lebedev.ri accepted this revision.Jul 2 2018, 9:22 AM

I don't have any notable feedback, so other than nits, looks good.
Maybe i'm missing something and you want to wait for a second review[er].

1158 ↗(On Diff #153707)

Would it be simpler to do

Value *OpV = Op0IsBinop ? Op0 : Op1
Value *OpC = Op0IsBinop ? Op1 : Op0

and use these later on?

1166 ↗(On Diff #153707)

Note: ConstantExpr::getBinOpIdentity() does not handle div/rem.

1170–1174 ↗(On Diff #153707)

Nit: i would like the comment to show an example here.

This revision is now accepted and ready to land.Jul 2 2018, 9:22 AM
spatel updated this revision to Diff 153820.Jul 2 2018, 4:11 PM

Patch updated:
No logic difference from the previous rev, just tried to improve the code structure/comments as suggested.
If there are no objections, I'll commit tomorrow.

This revision was automatically updated to reflect the committed changes.