[PGO] Avoid assertion on profile generated from code without an exit block

[wolfgangp]

Summary

A degenerate profile can cause an assertion at code gen time. Specifically, we have a non-zero basic block count with 0 counts in all of the successor blocks.

This has occurred in practice at least once, and while the circumstances under which the profile was created were not immediately reproducible, the crash can be avoided by guarding the offending call.

[davidxl]

This looks like related to infinite loops.

[xur]

In D134203#3800684, @davidxl wrote:

This looks like related to infinite loops.

Just found there is a test case in this patch. Let me take a look at why this is happening.

[xur]

I'm fine with change. But can you add a warning message here? I would be helpful to know something is wrong here.

[xur]

This is an interesting example. When we design this, we assume there is at least one exit node in the function. And we create fake edges from exit node(s). This way we create a close graph.

For this case, we don't have an exit node and I think the function exits from a non-return call to "zeroexit". So the edge propagation will terminate incompletely.

I think this is a corner case but let me think how to handle this.

[xur]

In D134203#3801212, @xur wrote:

This is an interesting example. When we design this, we assume there is at least one exit node in the function. And we create fake edges from exit node(s). This way we create a close graph.

For this case, we don't have an exit node and I think the function exits from a non-return call to "zeroexit". So the edge propagation will terminate incompletely.

I think this is a corner case but let me think how to handle this.

I read the code again and I think a closed graph is not required for the propagation. The flow will be inconsistent but there is little we can do about it.
The root cause is we don't break function call into a separated BB. So the implicit edges from non-return calls are not accountable.

We probably don't want to change this because it will slow down the instrumentation and the benefit is not minimal for PGO performance.

I think this fix is fine.

But there are a few minor things need to change
(1) the title: the problem is not from degenerate profile. I can create a prefect legal profile and reproduce the assertion
(2) add a waring or debug trace

You also probably want to use a reduce test case.
Here is the test case I created:

target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define void @bar(i32 noundef %s) {
entry:
  %cmp = icmp sgt i32 %s, 20
  br i1 %cmp, label %if.then, label %if.end

if.then:
  call void @exit(i32 noundef 1)
  unreachable

if.end:
  ret void
}

declare void @exit(i32 noundef)

define void @foo(i32 noundef %n) {
entry:
  %sum = alloca i32, align 4
  store volatile i32 %n, ptr %sum, align 4
  %sum.0.sum.0. = load volatile i32, ptr %sum, align 4
  call void @bar(i32 noundef %sum.0.sum.0.)
  %cmp = icmp slt i32 %n, 10
  br i1 %cmp, label %if.then, label %if.end

if.then:
  %sum.0.sum.0.1 = load volatile i32, ptr %sum, align 4
  call void @bar(i32 noundef %sum.0.sum.0.1)
  br label %if.end

if.end:
  br label %for.cond

for.cond:
  %sum.0.sum.0.2 = load volatile i32, ptr %sum, align 4
  call void @bar(i32 noundef %sum.0.sum.0.2)
  br label %for.cond
}

profile file:

# IR level Instrumentation Flag
:ir
foo
# Func Hash:
146835645050580305
# Num Counters:
3
# Counter Values:
0
1
0

[wolfgangp]

Sorry about the delay. Thank you for your test case, I have used it instead of the original one and added a warning.
Please check if the wording describes the issue reasonably correctly and succinctly, as the circumstances are somewhat arcane.

[wolfgangp]

ping ...

[xur]

Looks good to me.

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

for (unsigned s = 0; s < Size; s++) {
       if (DestBB == nullptr)
         continue;
       unsigned SuccNum = GetSuccessorNumber(SrcBB, DestBB);
       uint64_t EdgeCount = E->CountValue;
       if (EdgeCount > MaxCount)
         MaxCount = EdgeCount;
       EdgeCounts[SuccNum] = EdgeCount;
     }
+
+    if (MaxCount)
       setProfMetadata(M, TI, EdgeCounts, MaxCount);
+    else {
+      // A zero MaxCount can come about when we have a BB with a positive
+      // count, and whose successor blocks all have 0 count. This can happen
+      // when there is no exit block and the code exits via a noreturn function.
+      auto &Ctx = M->getContext();
+      Ctx.diagnose(DiagnosticInfoPGOProfile(
+        M->getName().data(),
+        Twine("Profile in ") + F.getName().str() +
+            Twine(" partially ignored") +
+            Twine(", possibly due to the lack of a return path."),
+        DS_Warning));
+    }
   }
 }
 
 static bool isIndirectBrTarget(BasicBlock *BB) {
   for (BasicBlock *Pred : predecessors(BB)) {
     if (isa<IndirectBrInst>(Pred->getTerminator()))
       return true;
   }

llvm/lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp

if (DT) {
       Updates.push_back({DominatorTree::Insert, CaseBB, MergeBB});
       Updates.push_back({DominatorTree::Insert, BB, CaseBB});
     }
     LLVM_DEBUG(dbgs() << *CaseBB << "\n");
   }
   DTU.applyUpdates(Updates);
   Updates.clear();
 
+  if (MaxCount)
     setProfMetadata(Func.getParent(), SI, CaseCounts, MaxCount);
 
   LLVM_DEBUG(dbgs() << *BB << "\n");
   LLVM_DEBUG(dbgs() << *DefaultBB << "\n");
   LLVM_DEBUG(dbgs() << *MergeBB << "\n");
 
   ORE.emit([&]() {
     using namespace ore;
     return OptimizationRemark(DEBUG_TYPE, "memopt-opt", MO.I)

llvm/test/Transforms/PGOProfile/Inputs/maxcountzero.proftext

+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+146835645050580305
+# Num Counters:
+3
+# Counter Values:
+0
+1
+0

llvm/test/Transforms/PGOProfile/maxcountzero.ll

+; Make sure a profile that is generated from a function without an exit node
+; does not cause an assertion. The profile consists of a non-zero count in a
+; basic block and 0 counts in all succcessor blocks. Expect a warning.
+
+; RUN: llvm-profdata merge %S/Inputs/maxcountzero.proftext -o %t.profdata
+; RUN: opt < %s -passes=pgo-instr-use -pgo-instrument-entry=false -pgo-test-profile-file=%t.profdata -S 2>&1 | FileCheck %s
+
+define void @bar(i32 noundef %s) {
+entry:
+  %cmp = icmp sgt i32 %s, 20
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  call void @exit(i32 noundef 1)
+  unreachable
+
+if.end:
+  ret void
+}
+
+declare void @exit(i32 noundef)
+
+define void @foo(i32 noundef %n) {
+entry:
+  %sum = alloca i32, align 4
+  store volatile i32 %n, ptr %sum, align 4
+  %sum.0.sum.0. = load volatile i32, ptr %sum, align 4
+  call void @bar(i32 noundef %sum.0.sum.0.)
+  %cmp = icmp slt i32 %n, 10
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  %sum.0.sum.0.1 = load volatile i32, ptr %sum, align 4
+  call void @bar(i32 noundef %sum.0.sum.0.1)
+  br label %if.end
+
+if.end:
+  br label %for.cond
+
+for.cond:
+  %sum.0.sum.0.2 = load volatile i32, ptr %sum, align 4
+  call void @bar(i32 noundef %sum.0.sum.0.2)
+  br label %for.cond
+}
+
+; CHECK: warning:{{.*}}Profile in foo partially ignored
+; CHECK: define