[PATCH] D136237: [BasicBlockSections] avoid insertting redundant branch to fall through blocks

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thanks for the revision.



================
Comment at: lld/test/ELF/lto/basic-block-sections-redundant-br.ll:1
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=all -O0 | FileCheck %s
+; This test case is generated from code:
----------------
Please add a one line comment, explaining the purpose of this test.


================
Comment at: lld/test/ELF/lto/basic-block-sections-redundant-br.ll:20-24
+; CHECK-NEXT: .cfi_startproc
+; CHECK-NEXT: # %bb.0: # %entry
+; CHECK-NEXT: movl {{.*}}
+; CHECK-NEXT: movl {{.*}}
+; CHECK-NEXT: subl {{.*}}
----------------
You don't need this since you have CHECKS for `mapping.__part.1` and `mapping.__part.2`. I suggest we just check the jumps.


================
Comment at: lld/test/ELF/lto/basic-block-sections-redundant-br.ll:27
+; CHECK-NEXT: jmp	mapping.__part.1
+; CHECK-NOT:  jmp	{{.*}}
+; CHECK: mapping.__part.1:
----------------
`CHECK-NOT:  jmp` is sufficient.



================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:125-126
 
+/// Basic blocks with an explicit unconditional branch to its fallthrough block
+/// do not need an extra unconditional branch.
+static bool hasUncoditionalBranch(const MachineBasicBlock &MBB) {
----------------
This comment seems to be explaining something about the context in which this function is called. I suggest we remove it from here.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:129
+  return !MBB.empty() &&
+         std::prev(MBB.terminators().end())->isUnconditionalBranch();
+}
----------------
Looking again, it seems that `terminators` is simply a range starting at the first terminator and ending at `MachineBasicBlock::end()`.https://llvm.org/doxygen/classllvm_1_1MachineBasicBlock.html#a7f0521fa2de44271fd4b909ea7351ef3 
So I think we can simply do:
`!MBB.empty() && MBB.back().isUnconditionalBranch()`.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:153-155
+    //        order, or
+    //     3- the terminator of the block is an unconditional branch to its
+    //        fallthrough.
----------------
This comment is incorrect. If it has an unconditional branch to its fallthrough, then we *don't* need an additional jump.


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

https://reviews.llvm.org/D136237