Page MenuHomePhabricator

[InstCombine] Add a few more patterns for folding select of select

Authored by aqjune on Apr 27 2021, 9:17 AM.



This is a patch that folds select of select to salvage some optimizations after select -> and/or folding is disabled.

select (select a, true, b), c, false -> select a, c, false
select c, (select a, true, b), false -> select c, a, false
  if c implies that b is false (isImpliedCondition).,

sel (sel c, a, false), true, (sel !c, b, false) -> sel c, a, b
sel (sel !c, a, false), true, (sel c, b, false) -> sel c, b, a,

See D101191

Diff Detail

Event Timeline

aqjune created this revision.Apr 27 2021, 9:17 AM
aqjune requested review of this revision.Apr 27 2021, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2021, 9:17 AM
nikic added inline comments.Apr 28 2021, 1:09 PM

This is pretty hard to understand, I think it will be more obvious if you match both patterns separately and use Builder.CreateSelect here instead of modifying all three operands in place.


Missing test for the second variant?

aqjune updated this revision to Diff 341416.Apr 29 2021, 12:00 AM

address comments

aqjune marked 2 inline comments as done.Apr 29 2021, 12:01 AM
aqjune added inline comments.

Hope this new code addresses the concern.

spatel added inline comments.Apr 29 2021, 10:38 AM

Don't need to use the Builder?

return SelectInst::Create(C1, A, B);
return SelectInst::Create(C2, B, A);
aqjune updated this revision to Diff 341723.Apr 29 2021, 5:38 PM
aqjune marked an inline comment as done.

Use SelectInst::Create

spatel added inline comments.Apr 30 2021, 5:47 AM

Better to make the type explicit here and the next block, so it is obvious:

Optional<bool> Res = isImpliedCondition...

How is this test different than the one above it?
Is the comment about complexity-based ordering copy/pasted here? I'm not seeing why it is necessary in these tests (could we just pass in an i1 argument to the function?).

aqjune updated this revision to Diff 342194.May 1 2021, 8:23 PM

Address comments

aqjune updated this revision to Diff 342195.May 1 2021, 8:25 PM
aqjune marked an inline comment as done.

minor update

aqjune marked 2 inline comments as done.May 1 2021, 8:25 PM
aqjune added inline comments.

Yes, it was redundant, I fixed it

nikic accepted this revision.May 2 2021, 1:45 AM


This revision is now accepted and ready to land.May 2 2021, 1:45 AM
This revision was landed with ongoing or failed builds.May 2 2021, 3:05 AM
This revision was automatically updated to reflect the committed changes.
aqjune marked an inline comment as done.