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

[Rahman Lavaee via Phabricator via llvm-commits]

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

Thanks again for the comments @tmsriram



================
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 (shouldEmitLabelForBasicBlock(MBB)) {
+    if (isVerbose() && MBB.hasLabelMustBeEmitted())
----------------
tmsriram wrote:
> Should we keep the same order as before? Any reason to flip it?  I don't have a strong opinion but the change is simpler.
Thanks for the comment. I think initially the condition was reversed due to the simpler logic of the "false" direction. Since we now use a helper function to extract the condition, we can do it the natural way. I can change this to `if (!shouldEmitLabelForBasicBlock(MBB))` and reverse the two directions, but don't see the value in that.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:3144
+/// MachineBasicBlock::getSymbol and may assume different names under various
+/// circumstances.
+bool AsmPrinter::shouldEmitLabelForBasicBlock(
----------------
tmsriram wrote:
> "assume different names under various circumstances" , please rephrase.
Removed the phrase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89423