[PGO] Don't unconditionally request BBInfo in verifyFuncBFI()

[aeubanks]

This breaks in the case where there are unreachable blocks after an entry block with no successors, which don't have a BBInfo, causing crashes.

BBInfo doesn't exist for unreachable blocks, see https://reviews.llvm.org/D27280.

Fixes #135828.

[llvmbot]

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Arthur Eubanks (aeubanks)

Changes

This breaks in the case where there are unreachable blocks after an entry block with no successors, which don't get visited in CFGMST, causing crashes.

Fixes #135828.

---

Full diff: https://github.com/llvm/llvm-project/pull/140804.diff

3 Files Affected:

• (modified) llvm/include/llvm/Transforms/Instrumentation/CFGMST.h (-6)
• (added) llvm/test/Transforms/PGOProfile/Inputs/unreachable-block.proftext (+9)
• (added) llvm/test/Transforms/PGOProfile/unreachable-block.ll (+12)

diff --git a/llvm/include/llvm/Transforms/Instrumentation/CFGMST.h b/llvm/include/llvm/Transforms/Instrumentation/CFGMST.h
index f6bf045f7de2c..6f0c63b888353 100644
--- a/llvm/include/llvm/Transforms/Instrumentation/CFGMST.h
+++ b/llvm/include/llvm/Transforms/Instrumentation/CFGMST.h
@@ -133,12 +133,6 @@ template <class Edge, class BBInfo> class CFGMST {
     LLVM_DEBUG(dbgs() << "  Edge: from fake node to " << Entry->getName()
                       << " w = " << EntryWeight << "\n");
 
-    // Special handling for single BB functions.
-    if (succ_empty(Entry)) {
-      addEdge(Entry, nullptr, EntryWeight);
-      return;
-    }
-
     static const uint32_t CriticalEdgeMultiplier = 1000;
 
     for (BasicBlock &BB : F) {
diff --git a/llvm/test/Transforms/PGOProfile/Inputs/unreachable-block.proftext b/llvm/test/Transforms/PGOProfile/Inputs/unreachable-block.proftext
new file mode 100644
index 0000000000000..9be174fb32bca
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/Inputs/unreachable-block.proftext
@@ -0,0 +1,9 @@
+# IR level Instrumentation Flag
+:ir
+foo
+# Func Hash:
+742261418966908927
+# Num Counters:
+1
+# Counter Values:
+1
diff --git a/llvm/test/Transforms/PGOProfile/unreachable-block.ll b/llvm/test/Transforms/PGOProfile/unreachable-block.ll
new file mode 100644
index 0000000000000..bd55185649989
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/unreachable-block.ll
@@ -0,0 +1,12 @@
+; RUN: llvm-profdata merge %S/Inputs/unreachable-block.proftext -o %t.profdata
+; RUN: opt < %s -passes=pgo-instr-use -pgo-test-profile-file=%t.profdata -S
+
+declare ptr @bar()
+
+define ptr @foo() {
+entry:
+  ret ptr null
+
+2:
+  ret ptr null
+}

[mtrofin]

Is the more general "unreachable BB" a case?

[aeubanks]

I've updated the PR and description based on more digging, ptal

[ellishg]

Should we emit a warning in this case? Why is it ok to silently ignore here?

[mingmingl-llvm]

In an FDO-optimized build, hash mismatch errors are detected and logged (

llvm-project/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

Lines 1431 to 1438 in 744a469

	std::string Msg =
	IPE.message() + std::string(" ") + F.getName().str() +
	std::string(" Hash = ") + std::to_string(FuncInfo.FunctionHash) +
	std::string(" up to ") + std::to_string(MismatchedFuncSum) +
	std::string(" count discarded");
	Ctx.diagnose(
	DiagnosticInfoPGOProfile(M->getName().data(), Msg, DS_Warning));

).

Is it feasible to use similar mechanism to log the error here?

[aeubanks]

only unreachable basic blocks don't have a BBInfo, those shouldn't matter

[hiraditya]

In that case, is it possible to assert that BB is unreachable?

[mtrofin]

I'm guessing DominatorTree wasn't (yet) computed in -O0 and he'd rather not compute it?

[mtrofin]

lgtm

one alternative would be to DCE unreachable BBs as a prelude to instrumentation. But this patch could be OK too.

I'm curious, how did we end up with unreachable BBs at this stage anyway? The early inliner?

[aeubanks]

I'm curious, how did we end up with unreachable BBs at this stage anyway? The early inliner?

-O0 :)

[hiraditya]

nice find btw!