[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 fix. Does this every trigger with `-O2`?



================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:133
+/// do not need an extra unconditional branch.
+static bool needInsertUncondBranch(const MachineBasicBlock &MBB) {
+  auto I = MBB.terminators().begin(), E = MBB.end();
----------------
I suggest we change the name to `hasUncoditionalBranch` and flip the return value and the condition below, to make it more readable.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:134-141
+  auto I = MBB.terminators().begin(), E = MBB.end();
+
+  while (I != E) {
+    if (I->isUnconditionalBranch())
+      return false;
+    I++;
+  }
----------------
Can we simply write:
` return !MBB.terminators.empty() && MBB.terminators.back().isUncoditionalBranch()`


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:163
+    if (FTMBB && (MBB.isEndSection() || &*NextMBBI != FTMBB) &&
+        needInsertUncondBranch(MBB))
       TII->insertUnconditionalBranch(MBB, FTMBB, MBB.findBranchDebugLoc());
----------------
Or simply change this to
`MBB.terminators.empty() || !MBB.terminators.back().isUnconditionalBranch()` and add a comment.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections_2.ll:21-25
+; REDUNDANT-LABEL: __cxx_global_var_init:
+; REDUNDANT: jmp __cxx_global_var_init.__part.2
+; REDUNDANT-NOT: jmp __cxx_global_var_init.__part.1
+; REDUNDANT-LABEL: __cxx_global_var_init.__part.1:
+; REDUNDANT-LABEL: __cxx_global_var_init.1:
----------------
Can we use a different file for this test? The file header describes a different purpose for this test. Also `__cxx_global_var_init` is an automatically-generated function. It would be better if we come up a with a standalone test with a normal function -- if possible.


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

https://reviews.llvm.org/D136237