[profcheck] Exclude naked, asm-only functions from profcheck

[mtrofin]

We can't do anything meaningful to such functions: they aren't optimizable, and even if inlined, they would bring no code open to optimization.

[mtrofin]

• [profcheck] Exclude naked, asm-only functions from profcheck #168447 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-pgo

@llvm/pr-subscribers-llvm-transforms

Author: Mircea Trofin (mtrofin)

Changes

We can't do anything meaningful to such functions: they aren't optimizable, and even if inlined, they would bring no code open to optimization.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/ProfileVerify.cpp (+19)
• (added) llvm/test/Transforms/PGOProfile/profcheck-exclusions.ll (+10)

diff --git a/llvm/lib/Transforms/Utils/ProfileVerify.cpp b/llvm/lib/Transforms/Utils/ProfileVerify.cpp
index c7cf8256d393c..ca6bd91b1f530 100644
--- a/llvm/lib/Transforms/Utils/ProfileVerify.cpp
+++ b/llvm/lib/Transforms/Utils/ProfileVerify.cpp
@@ -65,11 +65,26 @@ class ProfileInjector {
   ProfileInjector(Function &F, FunctionAnalysisManager &FAM) : F(F), FAM(FAM) {}
   bool inject();
 };
+
+bool isAsmOnly(const Function &F) {
+  if (!F.hasFnAttribute(Attribute::AttrKind::Naked))
+    return false;
+  for (const auto &BB : F)
+    for (const auto &I : drop_end(BB.instructionsWithoutDebug())) {
+      const auto *CB = dyn_cast<CallBase>(&I);
+      if (!CB || !CB->isInlineAsm())
+        return false;
+    }
+  return true;
+}
 } // namespace
 
 // FIXME: currently this injects only for terminators. Select isn't yet
 // supported.
 bool ProfileInjector::inject() {
+  // skip purely asm functions
+  if (isAsmOnly(F))
+    return false;
   // Get whatever branch probability info can be derived from the given IR -
   // whether it has or not metadata. The main intention for this pass is to
   // ensure that other passes don't drop or "forget" to update MD_prof. We do
@@ -176,6 +191,10 @@ PreservedAnalyses ProfileInjectorPass::run(Function &F,
 
 PreservedAnalyses ProfileVerifierPass::run(Function &F,
                                            FunctionAnalysisManager &FAM) {
+  // skip purely asm functions
+  if (isAsmOnly(F))
+    return PreservedAnalyses::all();
+
   const auto EntryCount = F.getEntryCount(/*AllowSynthetic=*/true);
   if (!EntryCount) {
     auto *MD = F.getMetadata(LLVMContext::MD_prof);
diff --git a/llvm/test/Transforms/PGOProfile/profcheck-exclusions.ll b/llvm/test/Transforms/PGOProfile/profcheck-exclusions.ll
new file mode 100644
index 0000000000000..a2420be4fac30
--- /dev/null
+++ b/llvm/test/Transforms/PGOProfile/profcheck-exclusions.ll
@@ -0,0 +1,10 @@
+; RUN: opt -passes=prof-inject %s -S -o - | FileCheck %s
+; RUN: opt -passes=prof-verify %s --disable-output
+
+
+define void @bar(i1 %c) #0 {
+  ret void
+}
+
+attributes #0 = { naked }
+; CHECK-NOT: !prof

[github-actions]

🐧 Linux x64 Test Results

• 186262 tests passed
• 4848 tests skipped

[pcc]

I would just check for the attribute, it's invalid to have anything other than asm in a naked function.

[mtrofin]

I thought so too initially, but then (1) read https://llvm.org/docs/LangRef.html which just says

This attribute disables prologue / epilogue emission for the function. This can have very system-specific consequences. The arguments of a naked function can not be referenced through IR values.

...which is more permissive; and (2) couldn't find any check in Verifier.cpp that's more than

  if (Attrs.hasFnAttr(Attribute::Naked))
    for (const Argument &Arg : F.args())
      Check(Arg.use_empty(), "cannot use argument of naked function", &Arg);

Maybe there was a subsequent discussion but no doc/verifier update? (Happy to do those separately)

[pcc]

That's just saying that some targets might accept more than asm (which I'm not sure is even true). Even if true, that still doesn't necessarily mean that it's valid to inline.

[boomanaiden154]

It doesn't sound like the extra checks hurt, and they might catch something weird some day.

[mtrofin]

I'll land with the expanded check, since the equivalence naked <-> asm only or other implications isn't clear. Easy to evolve later.

[mtrofin]

Merge activity

• Nov 20, 4:31 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Nov 20, 4:32 PM UTC: @mtrofin merged this pull request with Graphite.