This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] optimize conditional branch on CRSET/CRUNSET
ClosedPublic

Authored by inouehrs on Sep 21 2018, 2:45 AM.

Details

Summary

This patch adds a check to optimize conditional branch (BC and BCn) based on a constant set by CRSET or CRUNSET.
Other optimizers, such as block placement, may generate such code and hence I do this at the very end of the optimization in pre-emit peephole pass.

A conditional branch based on a constant is eliminated or converted into unconditional branch. Also CRSET/CRUNSET is also eliminated if the condition code register is not used by instruction other than the branch to be optimized.

I observed such redundancy happened around 150 times while building LLVM bootctrap.

Diff Detail

Repository
rL LLVM

Event Timeline

inouehrs created this revision.Sep 21 2018, 2:45 AM
inouehrs updated this revision to Diff 166426.Sep 21 2018, 2:54 AM

add -verify-machineinstrs options in unit tests

hfinkel accepted this revision.Sep 21 2018, 9:27 AM

LGTM

This revision is now accepted and ready to land.Sep 21 2018, 9:27 AM

I'm just curious, is there something that runs after this pass that will clean up unreachable blocks if any became unreachable? For example, if MBB1 is the only predecessor of MBB2 and MBB2 is removed as a successor to MBB1, will something remove MBB2 afterwards.

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
121 ↗(On Diff #166426)

Maybe add a comment to this assert such as "Non-terminator in BB after a terminator".

139 ↗(On Diff #166426)

Don't you mean successors in this comment?

inouehrs updated this revision to Diff 166711.Sep 24 2018, 10:07 AM
inouehrs updated this revision to Diff 166713.
inouehrs marked 2 inline comments as done.Sep 24 2018, 10:12 AM

I'm just curious, is there something that runs after this pass that will clean up unreachable blocks if any became unreachable? For example, if MBB1 is the only predecessor of MBB2 and MBB2 is removed as a successor to MBB1, will something remove MBB2 afterwards.

Actually, no pass can eliminate unreachable block from this optimization. So I added a simple unrechable block elimination and modify a testcase for this.

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
139 ↗(On Diff #166426)

Oops. Thank you for the good catch.

nemanjai added inline comments.Sep 24 2018, 2:22 PM
lib/Target/PowerPC/PPCPreEmitPeephole.cpp
109 ↗(On Diff #166713)

Any one of these successors now might have become unreachable, right? Should this just be a static recursive function?
Seems that it would be nice if we could just run a pass that eliminates unreachable blocks after this pass.

inouehrs marked an inline comment as done.Sep 24 2018, 8:49 PM
inouehrs added inline comments.
lib/Target/PowerPC/PPCPreEmitPeephole.cpp
109 ↗(On Diff #166713)

As long as I tested, I have never seen an unreachable block caused by this optimization. So I feel it is not warth paying too much cost for this.
How about commiting this patch without unreachable block elimination and investigate last unreachable block elimination opportunity (not limited to those caused by this optimization) as a separate patch?

inouehrs added inline comments.Sep 25 2018, 8:11 PM
lib/Target/PowerPC/PPCPreEmitPeephole.cpp
109 ↗(On Diff #166713)

@nemanjai What do you think on this approach?

nemanjai accepted this revision.Sep 26 2018, 2:36 AM

LGTM.

lib/Target/PowerPC/PPCPreEmitPeephole.cpp
109 ↗(On Diff #166713)

I think this is valuable information - in practice, this doesn't cause unreachable blocks very often (if ever). Let's leave it as is then, I am in favour of your approach.

This revision was automatically updated to reflect the committed changes.