[GCOV] Don't add a useless block in the entry

[calixte]

Summary

Currently, we split the entry block of a function to be sure to have at least one edge. In fact it's useless since we can add this edge in gcno/gcda without creating it.
There are several pros to do that:

• slightly reduce gcno/gcda size (close to nothing...).
• fix a crash when there are several CU attached to a module (the entry block was splitted several times)
• when using -femit-coverage-data the entry was not splitted since it's currently done when emitting coverage notes, so no instrumentation added when there was only on block.

[efriedma]

I'm not really familiar with the gcov file format, but this change looks reasonable.

lib/Transforms/Instrumentation/GCOVProfiling.cpp

670

succ_empty()

There are four possible terminators for a block with no successors: ret, unreachable, resume, and cleanupret. Writing good testcases for exception handling is probably hard, but it would be nice to at least ensure we generate something sane for a function that explicitly calls exit().

test/Transforms/GCOVProfiling/multiple-cus.ll

5

(Needs to be fixed.)

[marco-c]

This looks good to me, modulo the changes requested by efriedma (and the additional tests to write).

It might be useful to stress test the patch on our CI to see if there are corner cases we haven't thought of.

[marco-c]

lib/Transforms/Instrumentation/GCOVProfiling.cpp

558

Nit: I'd keep this the other way around, to have a smaller diff

lib/Transforms/Instrumentation/GCOVProfiling.cpp

 
 static bool isUsingScopeBasedEH(Function &F) {
   if (!F.hasPersonalityFn()) return false;
 
   EHPersonality Personality = classifyEHPersonality(F.getPersonalityFn());
   return isScopedEHPersonality(Personality);
 }
 
-static bool shouldKeepInEntry(BasicBlock::iterator It) {
-  if (isa<AllocaInst>(*It)) return true;
-  if (isa<DbgInfoIntrinsic>(*It)) return true;
-  if (auto *II = dyn_cast<IntrinsicInst>(It)) {
-    if (II->getIntrinsicID() == llvm::Intrinsic::localescape) return true;
-  }
-
-  return false;
-}
-
 void GCOVProfiler::emitProfileNotes() {
   NamedMDNode *CU_Nodes = M->getNamedMetadata("llvm.dbg.cu");
   if (!CU_Nodes) return;
 
   for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     // Each compile unit gets its own .gcno file. This means that whether we run
     // this pass over the original .o's as they're produced, or run it after
     // LTO, we'll generate the same .gcno files.
for (unsigned i = 0, e = CU_Nodes->getNumOperands(); i != e; ++i) {
     unsigned FunctionIdent = 0;
     for (auto &F : M->functions()) {
       DISubprogram *SP = F.getSubprogram();
       if (!SP) continue;
       if (!functionHasLines(F)) continue;
       // TODO: Functions using scope-based EH are currently not supported.
       if (isUsingScopeBasedEH(F)) continue;
 
-      // gcov expects every function to start with an entry block that has a
-      // single successor, so split the entry block to make sure of that.
-      BasicBlock &EntryBlock = F.getEntryBlock();
-      BasicBlock::iterator It = EntryBlock.begin();
-      while (shouldKeepInEntry(It))
-        ++It;
-      EntryBlock.splitBasicBlock(It);
-
       Funcs.push_back(make_unique<GCOVFunction>(SP, &F, &out, FunctionIdent++,
                                                 Options.UseCfgChecksum,
                                                 Options.ExitBlockBeforeBody));
       GCOVFunction &Func = *Funcs.back();
 
       for (auto &BB : F) {
         GCOVBlock &Block = Func.getBlock(&BB);
-        TerminatorInst *TI = BB.getTerminator();
-        if (int successors = TI->getNumSuccessors()) {
-          for (int i = 0; i != successors; ++i) {
-            Block.addEdge(Func.getBlock(TI->getSuccessor(i)));
-          }
-        } else if (isa<ReturnInst>(TI)) {
+        if (succ_begin(&BB) == succ_end(&BB)) {
+          // No successors.
           Block.addEdge(Func.getReturnBlock());
+        } else {
+          for (BasicBlock *Succ : successors(&BB)) {
+            Block.addEdge(Func.getBlock(Succ));
+          }
         }
 
         uint32_t Line = 0;
         for (auto &I : BB) {
           // Debug intrinsic locations correspond to the location of the
           // declaration, not necessarily any statements or expressions.
           if (isa<DbgInfoIntrinsic>(&I)) continue;
 
for (auto &F : M->functions()) {
       if (!functionHasLines(F)) continue;
       // TODO: Functions using scope-based EH are currently not supported.
       if (isUsingScopeBasedEH(F)) continue;
       if (!Result) Result = true;
 
       DenseMap<std::pair<BasicBlock *, BasicBlock *>, unsigned> EdgeToCounter;
       unsigned Edges = 0;
       for (auto &BB : F) {
-        TerminatorInst *TI = BB.getTerminator();
-        if (isa<ReturnInst>(TI)) {
+        if (succ_begin(&BB) == succ_end(&BB)) {
+          // No successors.
           EdgeToCounter[{&BB, nullptr}] = Edges++;
         } else {
-          for (BasicBlock *Succ : successors(TI)) {
+          for (BasicBlock *Succ : successors(&BB)) {
             EdgeToCounter[{&BB, Succ}] = Edges++;
           }
         }
       }
 
       ArrayType *CounterTy =
         ArrayType::get(Type::getInt64Ty(*Ctx), Edges);
       GlobalVariable *Counters =
for (auto &F : M->functions()) {
             Phi->addIncoming(EdgeCounter, Pred);
           }
 
           // Skip phis, landingpads.
           IRBuilder<> Builder(&*BB.getFirstInsertionPt());
           Value *Count = Builder.CreateLoad(Phi);
           Count = Builder.CreateAdd(Count, Builder.getInt64(1));
           Builder.CreateStore(Count, Phi);
+        }
 
-          TerminatorInst *TI = BB.getTerminator();
-          if (isa<ReturnInst>(TI)) {
-            auto It = EdgeToCounter.find({&BB, nullptr});
-            assert(It != EdgeToCounter.end());
-            const unsigned Edge = It->second;
-            Value *Counter =
-                Builder.CreateConstInBoundsGEP2_64(Counters, 0, Edge);
-            Value *Count = Builder.CreateLoad(Counter);
-            Count = Builder.CreateAdd(Count, Builder.getInt64(1));
-            Builder.CreateStore(Count, Counter);
-          }
-        }
-      }
-    }
+        if (succ_begin(&BB) == succ_end(&BB)) {
+          IRBuilder<> Builder(&*BB.getFirstInsertionPt());
 
     Function *WriteoutF = insertCounterWriteout(CountersBySP);
     Function *FlushF = insertFlush(CountersBySP);
 
     // Create a small bit of code that registers the "__llvm_gcov_writeout" to
     // be executed at exit and the "__llvm_gcov_flush" function to be executed
     // when "__gcov_flush" is called.
     FunctionType *FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);

test/Transforms/GCOVProfiling/Inputs/empty-function-1.ll

+define void @empty1() {
+  ret void
+}

test/Transforms/GCOVProfiling/Inputs/empty-function-2.ll

+define void @empty2() {
+  ret void
+}

test/Transforms/GCOVProfiling/multiple-cus.ll

+; RUN: opt -debugify %S/Inputs/empty-function-1.ll -o %t.1.bc
+; RUN: opt -debugify %S/Inputs/empty-function-2.ll -o %t.2.bc
+; RUN: llvm-link %t.1.bc %t.2.bc -o %t.lto.bc
+; RUN: opt -S -insert-gcov-profiling %t.lto.bc -o -
+; TODO: Check that the IR is correct.

test/Transforms/GCOVProfiling/return-block.ll

 !17 = distinct !DILexicalBlock(line: 7, column: 7, file: !1, scope: !4)
 !18 = !DILocation(line: 9, column: 1, scope: !4)
 
 ; There should be no destination edges for the exit block.
 ; CHECK: Block : 1 Counter : 0
 ; RETURN-LAST:       Destination Edges
 ; RETURN-SECOND-NOT: Destination Edges
 ; CHECK: Block : 2 Counter : 0
-; CHECK: Block : 4 Counter : 0
+; CHECK: Block : 3 Counter : 0
 ; RETURN-LAST-NOT: Destination Edges
 ; RETURN-SECOND:   Destination Edges
 ; CHECK-NOT: Block :