Page MenuHomePhabricator

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

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

Details

Summary

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).

https://alive2.llvm.org/ce/z/ANatjt, https://alive2.llvm.org/ce/z/rv8zTB

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

https://alive2.llvm.org/ce/z/U2kp-t, https://alive2.llvm.org/ce/z/bc88EE

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
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2733

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.

llvm/test/Transforms/InstCombine/select-safe-transforms.ll
168–180

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.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2733

Hope this new code addresses the concern.

spatel added inline comments.Apr 29 2021, 10:38 AM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2733

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
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2707

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

Optional<bool> Res = isImpliedCondition...
llvm/test/Transforms/InstCombine/select-safe-transforms.ll
144–145

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.
llvm/test/Transforms/InstCombine/select-safe-transforms.ll
144–145

Yes, it was redundant, I fixed it

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

LGTM

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.