Page MenuHomePhabricator

[Jump Threading] Unfold a select instruction that feeds a switch statement via a phi node
ClosedPublic

Authored by amehsan on Dec 4 2018, 4:57 AM.

Details

Reviewers
brzycki
sebpop
Summary

Currently when a select has a constant value in one branch and the select feeds a conditional branch (via a compare/ phi and compare) we unfold the select statement. This results in threading the conditional branch later on. Similar opportunity exists when a select (with a constant in one branch) feeds a switch (via a phi node). The patch unfolds select under this condition. A testcase is provided.

The patch is tested on two out of tree targets with internal benchmarks and also with testsuite on X86. There are several cases where this code unfolds a select in testsuite (when testing on X86) but none resulted in performance improvement or degradation.

Diff Detail

Event Timeline

amehsan created this revision.Dec 4 2018, 4:57 AM

Hi @amehsan , I think the patch looks mostly good but for a few small comments or variable names to enhance readability.

include/llvm/Transforms/Scalar/JumpThreading.h
146

This should probably be written as:

bool TryToUnfoldSelect(SwitchInst *SI, BasicBlock *BB);
lib/Transforms/Scalar/JumpThreading.cpp
1176

Style again:

if (SwitchInst *SI = dyn_cast<SwitchInst>(BB->getTerminator()))
  TryToUnfoldSelect(SI, BB);
2393–2459

Please add documentation here describing the requirements for the code shape checks before entering UnfoldSelectInstr. For example, it's not easy to discern Pred has an unconditional branch to BB without looking at the call sites and the function.

2430

Possibly CondPHI instead of SwVar?

2437

SI is a bit confusing to me. I think of it as the top-level select. This might be better named as PredSI.

2441

Could you add a little more to the comment discussing why we don't want to alter the select for the 2nd and 3rd check here?

test/Transforms/JumpThreading/select.ll
437

Is this check sufficient? There is also a change in land.lhs.true where we no longer contain the %spec.select statement and the terminator instruction changes from unconditional to conditional.

dmgreen added a subscriber: dmgreen.Dec 7 2018, 1:24 AM
amehsan marked 7 inline comments as done.Dec 7 2018, 10:34 AM

Thanks @brzycki. I believe I have addressed all your comments. Will upload the modified patch shortly.

amehsan updated this revision to Diff 177314.Dec 7 2018, 2:16 PM
brzycki accepted this revision.Dec 10 2018, 8:11 AM

Awesome, thanks for doing this @amehsan .

This revision is now accepted and ready to land.Dec 10 2018, 8:11 AM

Thanks @brzycki for careful review. I need to revive my commit access (not used it for two years now), then I will commit this.

@amehsan gentle ping. Have you resolved your push access?

@amehsan gentle ping. Have you resolved your push access?

I responded to this by email but since I don't yet see the response here: Sorry for the delay. Will try to repeat the test-suite run again and commit this week. Since some time has passed, wanted to be on the safe side.

amehsan closed this revision.Jan 14 2019, 11:06 AM

Committed in rL350931