[PATCH] D89423: Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty.
Rahman Lavaee via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 19 14:57:05 PDT 2020
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
More information about the llvm-commits
mailing list