Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2025
+29 −0
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions llvm/lib/Transforms/Utils/ProfileVerify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

@pcc pcc Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mtrofin mtrofin Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@pcc pcc Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@boomanaiden154 boomanaiden154 Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@mtrofin mtrofin Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Expand Down Expand Up @@ -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);
Expand Down
10 changes: 10 additions & 0 deletions llvm/test/Transforms/PGOProfile/profcheck-exclusions.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Loading