This is an archive of the discontinued LLVM Phabricator instance.

[JumpThreading] Unfold selects that depend on the same condition
ClosedPublic

Authored by pbarrio on Oct 11 2016, 8:33 AM.

Details

Summary

These are good candidates for jump threading. This enables later opts
(such as InstCombine) to combine instructions from the selects with
instructions out of the selects. SimplifyCFG will fold the select
again if unfolding wasn't worth it.

Patch by James Molloy and Pablo Barrio.

Diff Detail

Repository
rL LLVM

Event Timeline

pbarrio updated this revision to Diff 74260.Oct 11 2016, 8:33 AM
pbarrio retitled this revision from to [JumpThreading] Unfold selects that depend on the same condition.
pbarrio updated this object.
pbarrio added reviewers: reames, bkramer, jmolloy.
pbarrio added a subscriber: llvm-commits.

Hi Pablo,

I only have a couple of trivial comments, but I'm not able to approve this patch myself as I wrote part of it and because I'm not well enough versed in this pass to give the LGTM.

Adding another set of reviewers who hopefully will be able to.

James

include/llvm/Transforms/Scalar/JumpThreading.h
139 ↗(On Diff #74260)

functions should start with a lowercase letter.

lib/Transforms/Scalar/JumpThreading.cpp
2048 ↗(On Diff #74260)

Variables should start with an Uppercase letter.

James, thanks for the review and also for adding the new reviewers. I will change the lower/upper case problem with the next round of comments, when I get feedback from others.

sebpop edited edge metadata.Oct 18 2016, 1:45 PM

Please fix the few formating issues.
The patch looks good otherwise.

lib/Transforms/Scalar/JumpThreading.cpp
2030 ↗(On Diff #74260)

Please run clang-format on your patch.

2033 ↗(On Diff #74260)

Please add a continue statement here: there is nothing else executed in the loop.
This will make it easier to read the rest of the loop body.

2038 ↗(On Diff #74260)

Let's move this comment close to the loop that looks for selects using the same bool... down...

2039 ↗(On Diff #74260)

Also remove the "else" after adding the "continue" above.

You could even flip the if condition:

if (!I.getType()->isIntegerTy(1))
  continue;

making all the rest of the loop one less indentation level.

2041 ↗(On Diff #74260)

... down here.

2047 ↗(On Diff #74260)

No need for else after continue.

haicheng edited edge metadata.Oct 19 2016, 12:34 PM

The code looks good except the formatting issues.

lib/Transforms/Scalar/JumpThreading.cpp
1943–1953 ↗(On Diff #74260)

Since you refactor the code of expanding select, maybe you want to refactor the code here, too.

pbarrio updated this revision to Diff 75436.Oct 21 2016, 8:31 AM
pbarrio marked 8 inline comments as done.
pbarrio edited edge metadata.

Thanks all for the reviews. Could you please review the requested formatting changes and approve if appropriate?

sebpop accepted this revision.Oct 21 2016, 10:05 AM
sebpop edited edge metadata.

LGTM.

lib/Transforms/Scalar/JumpThreading.cpp
2017 ↗(On Diff #75436)

Function names should start with lower case.

This revision is now accepted and ready to land.Oct 21 2016, 10:05 AM
This revision was automatically updated to reflect the committed changes.