[PATCH] D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked 3 inline comments as done.
rahmanl added a comment.

Thank you for your helpful comments @tmsriram and @snehasish



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3092
   // Print the main label for the block.
-  if (MBB.pred_empty() ||
-      (!MF->hasBBLabels() && isBlockOnlyReachableByFallthrough(&MBB) &&
-       !MBB.isEHFuncletEntry() && !MBB.hasLabelMustBeEmitted())) {
+  if ((!MBB.isEntryBlock() && (MF->hasBBLabels() || MBB.isBeginSection())) ||
+      (!MBB.pred_empty() &&
----------------
snehasish wrote:
> tmsriram wrote:
> > I think this would require a lot more comments.  I see that you have flipped the if-else here, is there any reason you did that. Plus, there are a lot more checks than earlier.  Could you add  more comments that explain what is going on here?  Thanks.  
> +1 perhaps even consider refactoring it out into a static helper function.
I refactored this into a private member function. The reason is that isBlockOnlyReachableByFallthrough is a virtual member function.


================
Comment at: llvm/lib/CodeGen/MachineBasicBlock.cpp:270
+bool MachineBasicBlock::isEntryBlock() const {
+  return getParent()->begin() == getIterator();
+}
----------------
tmsriram wrote:
> Is this the same as getParent()->front() == this?
Yes, except with a `&`:
`&getParent()->front() == this`
I find the current version more readable.


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll:17
+; CHECK-SECTIONS:       .section .text,"ax", at progbits,unique,2
+; CHECK-SECTIONS-NEXT:  foo.2:       # %default
+}
----------------
snehasish wrote:
> Should we also check for the appropriate CFI directives?
I intentionally want to limit the test to checking labels and sections only. The reason is that the CFI directive of this block may be removed since it has no real instructions. However, I think that may be a later follow-up with a separate test. Please let me know if you have a good reason.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89423