Page MenuHomePhabricator

[GISel] Add new GISel combiners for G_SELECT

Authored by mkitzan on Jul 14 2020, 5:06 PM.



Patch adds two new GICombinerRules for G_SELECT. The rules include: combining selects with undef comparisons into their first selectee value, and to combine away selects with constant comparisons. Patch additionally adds a new combiner test for the AArch64 target to test these new G_SELECT combiner rules and the existing select_same_val combiner rule.

Diff Detail

Event Timeline

mkitzan created this revision.Jul 14 2020, 5:06 PM
arsenm added inline comments.

This may not be correct? See 1cf6f210a2ed87dcda2183fffd6f9aa17b5c493c

mkitzan updated this revision to Diff 278244.Jul 15 2020, 10:53 AM

Looks like my local build wasn't building all the targets (sorry, fixed now). The AMDGPU test failed as a result of moving the existing select_same_val combiner out of the identity_combines group. The diff update moves select_same_val back to identity_combines group. The update does not address @arsenm's comment (waiting on feedback from @craig.topper).

I think all you should need is an equivalent to the NotPoison check in 1cf6f210a2ed87dcda2183fffd6f9aa17b5c493c, plus appropriate testcases.

mkitzan updated this revision to Diff 278253.Jul 15 2020, 11:10 AM

Maybe? It's unclear to me what makes a ConstantExpr poison in the first place, and how combining with it would result in incorrect code ( hire rolling through).

arsenm added inline comments.Jul 21 2020, 8:20 AM

This matches InstSimplify, although I do wonder why the choice isn't to select the RHS in this case. Since we generally prefer to canonicalize lower complexity expressions to the RHS, it seems like that would generally be the cheaper choice. I do think this should match the InstSimplify behavior though


Probably should split this into its own patch since it's questionable?


I've been thinking GIMatchData has the annoying property where every combine that uses the same thing needs to define its own GIDefMatchData.

mkitzan added inline comments.Jul 22 2020, 5:14 PM

Sounds reasonable. Will split that combine off into another patch.

mkitzan edited the summary of this revision. (Show Details)Jul 22 2020, 5:55 PM
mkitzan updated this revision to Diff 279994.Jul 22 2020, 6:17 PM

Removed the combine for undef selectee value. Will open a separate patch to track that work.

arsenm added inline comments.Jul 23 2020, 1:51 PM

I'm thinking there's no reason this needs to distinguish the undef and constant cases with D84165. However, that's making me second guess whether accepting G_IMPLICIT_DEF as an ordinary constant is a good idea if it can trigger the potentially poison case

arsenm accepted this revision.Aug 18 2020, 10:18 AM

LGTM, although the undef and case can probably be merged

This revision is now accepted and ready to land.Aug 18 2020, 10:18 AM

Thanks for the reviews. I don't have commit access, would be appreciated if someone could commit on my behalf. Thanks!

mkitzan updated this revision to Diff 287841.Aug 25 2020, 11:48 PM
  • Rebased and resolved conflicts
  • Fixed test/CodeGen/AMDGPU/GlobalISel/postlegalizercombiner-select.mir to continue testing unmerge with same regs
arsenm accepted this revision.Aug 27 2020, 6:50 AM

I can commit for you if no one has gotten to it yet.

mkitzan added a comment.EditedAug 27 2020, 9:02 AM

It was conflicted until the rebase update, so I doubt it was committed. Thanks for committing this!