Explicitly check for entry basic block, rather than relying on MachineBasicBlock::pred_empty.

[rahmanl]

Summary

Sometimes in unoptimized code, we have dangling unreachable basic blocks with no predecessors. Basic block sections should be emitted for those as well. Without this patch, the included test fails with a fatal error in AsmPrinter::emitBasicBlockEnd.

[tmsriram]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

This is the only place that EmitMBBlabel is checked. I don't see why you can't put that condition just directly after the if?

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll

4

There are no CHECK: statements?

[rahmanl]

• Fix the condition for emitting basic block label.

[rahmanl]

• Remove redundant CHECK check-prefix.

[rahmanl]

Thanks for the comments @tmsriram.

[tmsriram]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

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.

llvm/lib/CodeGen/MachineBasicBlock.cpp

270

Is this the same as getParent()->front() == this?

[snehasish]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

+1 perhaps even consider refactoring it out into a static helper function.

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll

18

Should we also check for the appropriate CFI directives?

[rahmanl]

• Add a private member function shouldEmitLabelForBasicBlock and call it to decide if basic blocks require symbols.

[rahmanl]

Thank you for your helpful comments @tmsriram and @snehasish

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

I refactored this into a private member function. The reason is that isBlockOnlyReachableByFallthrough is a virtual member function.

llvm/lib/CodeGen/MachineBasicBlock.cpp

270

Yes, except with a &:
&getParent()->front() == this
I find the current version more readable.

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll

18

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.

[tmsriram]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

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.

3179

3181

"assume different names under various circumstances" , please rephrase.

3184

3185

3189

[rahmanl]

• Better comments.

[rahmanl]

Thanks again for the comments @tmsriram

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3129–3133

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.

3181

Removed the phrase.

[tmsriram]

LGTM, thanks.

[MaskRay]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3178

The coding standard does not replicate the function name for a comment. Some old code uses does not conform to the standard

[rahmanl]

• Remove function comment for AsmPrinter::shouldEmitLabelForBasicBlock.

[rahmanl]

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

3178

Removed the comment.

llvm/include/llvm/CodeGen/AsmPrinter.h

private:
   /// Emit llvm.ident metadata in an '.ident' directive.
   void emitModuleIdents(Module &M);
   /// Emit bytes for llvm.commandline metadata.
   void emitModuleCommandLines(Module &M);
 
   GCMetadataPrinter *GetOrCreateGCPrinter(GCStrategy &S);
   /// Emit GlobalAlias or GlobalIFunc.
   void emitGlobalIndirectSymbol(Module &M, const GlobalIndirectSymbol &GIS);
+
+  /// This method decides whether the specified basic block requires a label.
+  bool shouldEmitLabelForBasicBlock(const MachineBasicBlock &MBB) const;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_ASMPRINTER_H

llvm/include/llvm/CodeGen/MachineBasicBlock.h

#endif
   bool isEHPad() const { return IsEHPad; }
 
   /// Indicates the block is a landing pad.  That is this basic block is entered
   /// via an exception handler.
   void setIsEHPad(bool V = true) { IsEHPad = V; }
 
   bool hasEHPadSuccessor() const;
 
+  /// Returns true if this is the entry block of the function.
+  bool isEntryBlock() const;
+
   /// Returns true if this is the entry block of an EH scope, i.e., the block
   /// that used to have a catchpad or cleanuppad instruction in the LLVM IR.
   bool isEHScopeEntry() const { return IsEHScopeEntry; }
 
   /// Indicates if this is the entry block of an EH scope, i.e., the block that
   /// that used to have a catchpad or cleanuppad instruction in the LLVM IR.
   void setIsEHScopeEntry(bool V = true) { IsEHScopeEntry = V; }
 

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::emitBBAddrMapSection(const MachineFunction &MF) {
   OutStreamer->PushSection();
   OutStreamer->SwitchSection(BBAddrMapSection);
   OutStreamer->emitSymbolValue(FunctionSymbol, getPointerSize());
   // Emit the total number of basic blocks in this function.
   OutStreamer->emitULEB128IntValue(MF.size());
   // Emit BB Information for each basic block in the funciton.
   for (const MachineBasicBlock &MBB : MF) {
     const MCSymbol *MBBSymbol =
-        MBB.pred_empty() ? FunctionSymbol : MBB.getSymbol();
+        MBB.isEntryBlock() ? FunctionSymbol : MBB.getSymbol();
     // Emit the basic block offset.
     emitLabelDifferenceAsULEB128(MBBSymbol, FunctionSymbol);
     // Emit the basic block size. When BBs have alignments, their size cannot
     // always be computed from their offsets.
     emitLabelDifferenceAsULEB128(MBB.getEndSymbol(), MBBSymbol);
     OutStreamer->emitULEB128IntValue(getBBAddrMapMetadata(MBB));
   }
   OutStreamer->PopSection();
void AsmPrinter::emitBasicBlockStart(const MachineBasicBlock &MBB) {
   // Emit an alignment directive for this block, if needed.
   const Align Alignment = MBB.getAlignment();
   if (Alignment != Align(1))
     emitAlignment(Alignment);
 
   // Switch to a new section if this basic block must begin a section. The
   // entry block is always placed in the function section and is handled
   // separately.
-  if (MBB.isBeginSection() && !MBB.pred_empty()) {
+  if (MBB.isBeginSection() && !MBB.isEntryBlock()) {
     OutStreamer->SwitchSection(
         getObjFileLowering().getSectionForMachineBasicBlock(MF->getFunction(),
                                                             MBB, TM));
     CurrentSectionBeginSym = MBB.getSymbol();
   }
 
   // If the block has its address taken, emit any labels that were used to
   // reference the block.  It is possible that there is more than one label
if (const BasicBlock *BB = MBB.getBasicBlock()) {
       }
     }
 
     assert(MLI != nullptr && "MachineLoopInfo should has been computed");
     emitBasicBlockLoopComments(MBB, MLI, *this);
   }
 
   // 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())
+      OutStreamer->AddComment("Label of block must be emitted");
+    OutStreamer->emitLabel(MBB.getSymbol());
+  } else {
     if (isVerbose()) {
       // NOTE: Want this comment at start of line, don't emit with AddComment.
       OutStreamer->emitRawComment(" %bb." + Twine(MBB.getNumber()) + ":",
                                   false);
     }
-  } else {
-    if (isVerbose() && MBB.hasLabelMustBeEmitted())
-      OutStreamer->AddComment("Label of block must be emitted");
-    OutStreamer->emitLabel(MBB.getSymbol());
   }
 
   // With BB sections, each basic block must handle CFI information on its own
   // if it begins a section (Entry block is handled separately by
   // AsmPrinterHandler::beginFunction).
-  if (MBB.isBeginSection() && !MBB.pred_empty())
+  if (MBB.isBeginSection() && !MBB.isEntryBlock())
     for (const HandlerInfo &HI : Handlers)
       HI.Handler->beginBasicBlock(MBB);
 }
 
 void AsmPrinter::emitBasicBlockEnd(const MachineBasicBlock &MBB) {
   // Check if CFI information needs to be updated for this MBB with basic block
   // sections.
   if (MBB.isEndSection())
case GlobalValue::ProtectedVisibility:
     Attr = MAI->getProtectedVisibilityAttr();
     break;
   }
 
   if (Attr != MCSA_Invalid)
     OutStreamer->emitSymbolAttribute(Sym, Attr);
 }
 
+bool AsmPrinter::shouldEmitLabelForBasicBlock(
+    const MachineBasicBlock &MBB) const {
+  // With `-fbasic-block-sections=`, a label is needed for every non-entry block
+  // in the labels mode (option `=labels`) and every section beginning in the
+  // sections mode (`=all` and `=list=`).
+  if ((MF->hasBBLabels() || MBB.isBeginSection()) && !MBB.isEntryBlock())
+    return true;
+  // A label is needed for any block with at least one predecessor (when that
+  // predecessor is not the fallthrough predecessor, or if it is an EH funclet
+  // entry, or if a label is forced).
+  return !MBB.pred_empty() &&
+         (!isBlockOnlyReachableByFallthrough(&MBB) || MBB.isEHFuncletEntry() ||
+          MBB.hasLabelMustBeEmitted());
+}
+
 /// isBlockOnlyReachableByFallthough - Return true if the basic block has
 /// exactly one predecessor and the control transfer mechanism between
 /// the predecessor and this block is a fall-through.
 bool AsmPrinter::
 isBlockOnlyReachableByFallthrough(const MachineBasicBlock *MBB) const {
-  // With BasicBlock Sections, beginning of the section is not a fallthrough.
-  if (MBB->isBeginSection())
-    return false;
-
   // If this is a landing pad, it isn't a fall through.  If it has no preds,
   // then nothing falls through to it.
   if (MBB->isEHPad() || MBB->pred_empty())
     return false;
 
   // If there isn't exactly one predecessor, it can't be a fall through.
   if (MBB->pred_size() > 1)
     return false;

llvm/lib/CodeGen/MachineBasicBlock.cpp

 
 bool MachineBasicBlock::hasEHPadSuccessor() const {
   for (const_succ_iterator I = succ_begin(), E = succ_end(); I != E; ++I)
     if ((*I)->isEHPad())
       return true;
   return false;
 }
 
+bool MachineBasicBlock::isEntryBlock() const {
+  return getParent()->begin() == getIterator();
+}
+
 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 LLVM_DUMP_METHOD void MachineBasicBlock::dump() const {
   print(dbgs());
 }
 #endif
 
 bool MachineBasicBlock::mayHaveInlineAsmBr() const {
   for (const MachineBasicBlock *Succ : successors()) {

llvm/test/CodeGen/X86/basic-block-sections-unreachable.ll

+; Check that basic block section is emitted when a non-entry block has no predecessors.
+; RUN: llc < %s -mtriple=x86_64 -O0 -basic-block-sections=all | FileCheck %s --check-prefix=CHECK-SECTIONS
+; RUN: llc < %s -mtriple=x86_64 -O0 | FileCheck %s --check-prefix=CHECK-NOSECTIONS
+define void @foo(i32* %bar) {
+  %v = load i32, i32* %bar
+  switch i32 %v, label %default [
+    i32 0, label %target
+  ]
+target:
+  ret void
+;; This is the block which will not have any predecessors. If the block is not garbage collected, it must
+;; be placed in a basic block section with a corresponding symbol.
+default:
+  unreachable
+; CHECK-NOSECTIONS:     # %bb.2:     # %default
+; CHECK-SECTIONS:       .section .text,"ax",@progbits,unique,2
+; CHECK-SECTIONS-NEXT:  foo.2:       # %default
+}