[PATCH] D134557: [BranchRelaxation] Fall through only if block has no unconditional branches

[Anshil Gandhi via Phabricator via llvm-commits]

gandhi21299 added a comment.

In D134557#3843510 <https://reviews.llvm.org/D134557#3843510>, @efriedma wrote:

> The logic here has gotten more confusing, not less.  Please just don't call getFallThrough; you can directly reason about the result of analyzeBranch.
>
> From TargetInstrInfo.h:
>
>   /// 1. If this block ends with no branches (it just falls through to its succ)
>   ///    just return false, leaving TBB/FBB null.
>   /// 2. If this block ends with only an unconditional branch, it sets TBB to be
>   ///    the destination block.
>   /// 3. If this block ends with a conditional branch and it falls through to a
>   ///    successor block, it sets TBB to be the branch destination block and a
>   ///    list of operands that evaluate the condition. These operands can be
>   ///    passed to other TargetInstrInfo methods to create new branches.
>   /// 4. If this block ends with a conditional branch followed by an
>   ///    unconditional branch, it returns the 'true' destination in TBB, the
>   ///    'false' destination in FBB, and a list of operands that evaluate the
>   ///    condition.  These operands can be passed to other TargetInstrInfo
>   ///    methods to create new branches.
>
> 1 is fallthrough.  2 is not fallthrough.  3 is fallthrough.  4 is not fallthrough.
>
> --------
>
> Not sure I understand the logic here when analyzeBranch fails; you just assume the block doesn't fall through?

analyzeBranch returns false when it //is// able to infer the terminators of the basic block. That is, determine TBB and FBB. This patch effectively leverages the cases for fallthrough as you mentioned for a specific case - only fall through for cases 1 and 3. Albeit I agree that not using getFallthrough() makes the code simpler and probably should not be used as the case already asserts that a fall through certainly exists for PrevBB (PrevBB is defined to be the block immediately prior to DestBB).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134557/new/

https://reviews.llvm.org/D134557