This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] eliminate unconditional branch to the next instruction
ClosedPublic

Authored by inouehrs on Sep 12 2017, 12:56 AM.

Details

Summary

This patch makes analyzeBranch eliminate unconditional branch to the next instruction.

After basic blocks are re-organized by optimizers, such as machine block placement, a BB may end with an unconditional branch to the next (fallthrough) BB. This patch removes such redundant branch instruction.

Diff Detail

Event Timeline

inouehrs created this revision.Sep 12 2017, 12:56 AM
nemanjai added inline comments.Sep 14 2017, 12:29 AM
lib/Target/PowerPC/PPCInstrInfo.cpp
499

This makes the assumption that the blocks are ordered in in the MachineFunction in the same order in which they'll be emitted. Can you add a comment with a quick explanation of why you're allowed to make that assumption.

500
  1. s/end/ends
  2. s/we eliminate it/we eliminate the branch instruction

The reason for 2. is that there's a bit of confusion of the subject of the sentence - it sounds like we're removing the basic block rather than the branch instruction.

echristo edited edge metadata.

Seems like we might want to have block layout clean up this sort of branch rather than analyzeBranch since we could theoretically make the same change in every backend?

inouehrs updated this revision to Diff 116564.Sep 25 2017, 9:20 AM
  • updated comments based on Nemanja's suggestions
  • rebased to the latest
inouehrs marked an inline comment as done.Sep 25 2017, 9:23 AM
inouehrs added inline comments.
lib/Target/PowerPC/PPCInstrInfo.cpp
499

I added a comment.
I am assuming that we can always find fallthrough BB based on MachineFunction::iterator in machine IR. Is there any case that we cannot assume so?

500

Thanks for the suggestion.

inouehrs updated this revision to Diff 116566.Sep 25 2017, 9:34 AM
inouehrs marked an inline comment as done.
  • updated by using isLayoutSuccessor to find fallthrough block.
inouehrs marked 2 inline comments as done.Sep 25 2017, 9:36 AM

Seems like we might want to have block layout clean up this sort of branch rather than analyzeBranch since we could theoretically make the same change in every backend?

I think this optimization should be machine dependent based on the following comment in MachineBlockPlacement.cpp. X86 backend already implements the same optimization.

// Indeed, the target may be able to optimize the branches in a way we
// cannot because all branches may not be analyzable.
// E.g., the target may be able to remove an unconditional branch to
// a fallthrough when it occurs after predicated terminators.
lib/Target/PowerPC/PPCInstrInfo.cpp
499

I rewrote the code by using a helper function, MachineBasicBlock::isLayoutSuccessor.

hfinkel accepted this revision.Sep 26 2017, 7:47 AM

Seems like we might want to have block layout clean up this sort of branch rather than analyzeBranch since we could theoretically make the same change in every backend?

I think this optimization should be machine dependent based on the following comment in MachineBlockPlacement.cpp. X86 backend already implements the same optimization.

// Indeed, the target may be able to optimize the branches in a way we
// cannot because all branches may not be analyzable.
// E.g., the target may be able to remove an unconditional branch to
// a fallthrough when it occurs after predicated terminators.

LGTM

This revision is now accepted and ready to land.Sep 26 2017, 7:47 AM
This revision was automatically updated to reflect the committed changes.
inouehrs marked an inline comment as done.