This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Fold another Select with And/Or pattern
ClosedPublic

Authored by xbolva00 on Jul 30 2018, 1:46 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

xbolva00 created this revision.Jul 30 2018, 1:46 AM
xbolva00 edited the summary of this revision. (Show Details)Jul 30 2018, 1:47 AM
xbolva00 added a reviewer: lebedev.ri.
xbolva00 retitled this revision from [InstSimplify] Fold another And/Or pattern to [InstSimplify] Fold another Select withAnd/Or pattern.Jul 30 2018, 3:01 AM
xbolva00 retitled this revision from [InstSimplify] Fold another Select withAnd/Or pattern to [InstSimplify] Fold another Select with And/Or pattern.Jul 30 2018, 3:22 AM
xbolva00 updated this revision to Diff 157934.Jul 30 2018, 3:35 AM
  • More tests

Any test missing? If ok, I will precommit them.

Any test missing? If ok, I will precommit them.

Tests look good; precommit.

The code logic seems unnecessarily complicated with all of these commuted matches. It would probably be easier to read and shorter if you match a binop with icmp operands and capture all 4 of those operands. Then, make sure the predicates match and make sure there's a common operand from the 4 captured icmp values.

xbolva00 updated this revision to Diff 158023.EditedJul 30 2018, 11:25 AM
  • Rebased

I will change the code logic tomorrow.

xbolva00 updated this revision to Diff 158178.Jul 30 2018, 11:51 PM
  • Updated code logic
xbolva00 updated this revision to Diff 158180.Jul 30 2018, 11:58 PM
spatel added inline comments.Jul 31 2018, 5:27 AM
lib/Analysis/InstructionSimplify.cpp
100 ↗(On Diff #158180)

I think the logic is correct, but this is confusing variable naming because it doesn't match the code comments above. Just call these *X, *Y?

103 ↗(On Diff #158180)

This does not need to be a commutative matcher?

xbolva00 updated this revision to Diff 158227.Jul 31 2018, 5:46 AM
  • Fixed review notes
xbolva00 marked 2 inline comments as done.Jul 31 2018, 5:46 AM
spatel accepted this revision.Jul 31 2018, 7:00 AM

LGTM with 1 more comment fix.

lib/Analysis/InstructionSimplify.cpp
68–82 ↗(On Diff #158227)

The comment still doesn't match the code. Might as well move this next to the match statement inside the function in case there are other folds possible here:

%A = icmp eq %TV, %FV
%B = icmp eq %X, %Y (and one of these is a select operand)
%C = and %A, %B
%D = select %C, %TV, %FV
-->
%FV

%A = icmp ne %TV, %FV
%B = icmp ne %X, %Y (and one of these is a select operand)
%C = or %A, %B
%D = select %C, %TV, %FV
-->
%TV
This revision is now accepted and ready to land.Jul 31 2018, 7:00 AM
xbolva00 updated this revision to Diff 158248.Jul 31 2018, 7:06 AM
  • Comment fix
xbolva00 updated this revision to Diff 158251.Jul 31 2018, 7:16 AM
This revision was automatically updated to reflect the committed changes.