This is an archive of the discontinued LLVM Phabricator instance.

[MachineBasicBlock] Explicit FT branching param
ClosedPublic

Authored by gandhi21299 on Dec 30 2022, 3:20 PM.

Details

Summary

Introduce a parameter in getFallThrough() to optionally
allow returning the fall through basic block in spite of
an explicit branch instruction to it. This parameter is
set to false by default.

Introduce getLogicalFallThrough() which calls
getFallThrough(false) to obtain the block while avoiding
insertion of a jump instruction to its immediate successor.

This patch also reverts the changes made by D134557 and
solves the case where a jump is inserted after another jump
(branch-relax-no-terminators.mir).

Diff Detail

Event Timeline

gandhi21299 created this revision.Dec 30 2022, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2022, 3:20 PM
gandhi21299 requested review of this revision.Dec 30 2022, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2022, 3:20 PM
gandhi21299 edited the summary of this revision. (Show Details)Dec 31 2022, 1:07 AM
arsenm added inline comments.Dec 31 2022, 7:46 AM
llvm/lib/CodeGen/BranchRelaxation.cpp
499–503

Can you factor this into something that looks like MachineBasicBlock::isReturnBlock, except for indirect branches?

gandhi21299 added inline comments.Dec 31 2022, 1:19 PM
llvm/lib/CodeGen/BranchRelaxation.cpp
499–503

Doesn't look like MachineBasicBlock has any method for indirect branches.

applied clang-format

efriedma added inline comments.Jan 4 2023, 10:05 AM
llvm/lib/CodeGen/BranchRelaxation.cpp
501

This condition doesn't seem right.

The point of the existing code is to handle fallthrough from PrevBB to DestBB: we're inserting a block between those two blocks, so any fallthrough must be rewritten to a direct branch. We can only skip this if we can prove there isn't any fallthrough.

isIndirectBranch() doesn't prove there isn't any fallthrough; whether a branch is indirect isn't related to whether it's conditional. MachineBasicBlock::canFallThrough() is probably appropriate.

  • Use getFallThrough() instead of checking for indirect branches.
  • Revert changes from D134557 and use an updated getFallThrough(..) where a param is set to true/false for explicit branching to fallthrough basic block. Allow explicit branching to fallthrough by default.
  • Apply clang-format
gandhi21299 retitled this revision from [BranchRelaxation] Prevent analyzing indirectBr during uncondBr fixup to [MachineBasicBlock] Explicit FT branching param.Jan 4 2023, 3:32 PM
gandhi21299 edited the summary of this revision. (Show Details)
gandhi21299 marked 2 inline comments as done.Jan 6 2023, 9:37 AM
gandhi21299 added inline comments.Jan 11 2023, 10:06 AM
llvm/lib/CodeGen/MachineBasicBlock.cpp
978–979

Thoughts on this change? @arsenm @efriedma

arsenm added inline comments.Jan 11 2023, 10:09 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
798

ExplicitFT isn't the right name. It's an unconditional branch. Introduce another name, maybe getFallthroughOrUnconditionalSuccessor?

gandhi21299 added inline comments.Jan 11 2023, 10:16 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
798

Sorry, do you mean for the method name or the parameter?

arsenm added inline comments.Jan 11 2023, 11:51 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
798

Separate method name, the parameter also should be something else if you leave it here as an implementation detail

  • Renamed the method and parameter name, as requested by reviewer
  • Applied clang-format
foad added inline comments.Jan 12 2023, 1:42 AM
llvm/test/CodeGen/AMDGPU/branch-relax-indirect-branch.mir
2

What is this new test testing? If it's a change in behaviour caused by your patch then please (a) precommit the test and (b) update the summary to explain what the change is.

gandhi21299 added inline comments.Jan 12 2023, 10:01 AM
llvm/test/CodeGen/AMDGPU/branch-relax-indirect-branch.mir
2

Do you mean to commit a patch which only contains the changes in branch-relax-indirect-branch.mir? That would cause this test to fail.

Before this patch, getFallThrough() was essentially expanded in the branch relaxation pass which fixed a double terminator bug (D134557). As a result, an indirect branch instruction caused SIInstrInfo::analyzeBranchImpl(..) to return true which further reported a fatal error. This patch reverts changes made by D134557 and fixes both issues by introducing a parameter to the method MachineBasicBlock::getFallThroughOrUnonditionalSuccessor(..).

gandhi21299 edited the summary of this revision. (Show Details)Jan 12 2023, 10:05 AM
foad added a comment.Jan 12 2023, 10:12 AM

The summary still doesn't explain that there was a bug, and how you fixed it. It makes it sound like you have just tweaked the API of some helper function.

gandhi21299 edited the summary of this revision. (Show Details)Jan 12 2023, 10:23 AM
gandhi21299 marked 2 inline comments as done.Jan 13 2023, 8:57 AM

ping

arsenm added inline comments.Jan 13 2023, 9:28 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
799

getFallThrough should exist as it did before, the other uses should not need updating

gandhi21299 added inline comments.Jan 13 2023, 9:41 AM
llvm/include/llvm/CodeGen/MachineBasicBlock.h
799

How does MachineBasicBlock::getFallThrough(bool AllowUncondSucc = true) sound?

Renamed getFallThroughOrUnconditionalSuccessor(..) back to getFallThrough(..)

gandhi21299 marked 2 inline comments as done.Jan 17 2023, 10:21 AM
gandhi21299 added a reviewer: foad.
gandhi21299 added a project: Restricted Project.Jan 17 2023, 11:52 AM
arsenm accepted this revision.Jan 17 2023, 12:57 PM
arsenm added inline comments.
llvm/include/llvm/CodeGen/MachineBasicBlock.h
799

I was looking for that as an implementation detail with two separate method names for the two behaviors

This revision is now accepted and ready to land.Jan 17 2023, 12:57 PM
gandhi21299 marked an inline comment as done.Jan 17 2023, 1:32 PM

Thanks for the review, I will commit this patch.

gandhi21299 edited the summary of this revision. (Show Details)Jan 17 2023, 1:33 PM
gandhi21299 edited the summary of this revision. (Show Details)Jan 17 2023, 1:39 PM
  • Added getLogicalFallThrough() which calls getFallThrough(true).
  • Renamed the param for getFallThrough().
gandhi21299 edited the summary of this revision. (Show Details)Jan 17 2023, 3:43 PM
arsenm accepted this revision.Jan 17 2023, 4:02 PM
This revision was landed with ongoing or failed builds.Jan 17 2023, 4:12 PM
This revision was automatically updated to reflect the committed changes.
/// Return the fallthrough block if the block can implicitly
/// transfer control to the block after it by falling off the end of
/// it. If an explicit branch to the fallthrough block is not allowed,
/// set JumpToFallThrough to be false. Non-null return is a conservative
/// answer.
MachineBasicBlock *getFallThrough(bool JumpToFallThrough = false);

The documentation for this function seems at odds with the behavior.

The documentation suggests that, if JumpToFallThrough is false, then getFallThrough will return a nullptr if the basic block ends with an explicit branch to the fallthrough block.

However, the opposite is true. For a block that ends in an unconditional jump to the fallthrough block, if JumpToFallThrough == true, then the function will return nullptr. If JumpToFallThrough == false, then it will return the address of the fallthrough block.

Is this the intended behavior?

/// Return the fallthrough block if the block can implicitly
/// transfer control to the block after it by falling off the end of
/// it. If an explicit branch to the fallthrough block is not allowed,
/// set JumpToFallThrough to be false. Non-null return is a conservative
/// answer.
MachineBasicBlock *getFallThrough(bool JumpToFallThrough = false);

The documentation for this function seems at odds with the behavior.

The documentation suggests that, if JumpToFallThrough is false, then getFallThrough will return a nullptr if the basic block ends with an explicit branch to the fallthrough block.

However, the opposite is true. For a block that ends in an unconditional jump to the fallthrough block, if JumpToFallThrough == true, then the function will return nullptr. If JumpToFallThrough == false, then it will return the address of the fallthrough block.

Is this the intended behavior?

You're right! I will revert this patch and re-merge it with the corrected documentation.