Skip to content

[PartiallyInlineLibCalls] Emit missed- and passed-optimization remarks when partially inlining sqrt #123966

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[PartiallyInlineLibCalls] Emit missed- and passed-optimization remarks when partially inlining sqrt #123966

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
12e8f4a
Add passed and missed opt-remarks to PartiallyInlineLibCalls
Jan 22, 2025
fb45df1
Messages now start with lowercase
TiborGY May 17, 2025
ce81d5c
Move analysis remark about fast sqrt isn next to the condition that c…
TiborGY May 18, 2025
1285745
Use FileCheck's --input-file option instead of cat|
TiborGY May 18, 2025
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
38 changes: 35 additions & 3 deletions llvm/lib/Transforms/Scalar/PartiallyInlineLibCalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,20 @@ static bool optimizeSQRT(CallInst *Call, Function *CalledFunc,
// dst = phi(v0, v1)
//

ORE->emit([&]() {
return OptimizationRemark(DEBUG_TYPE, "SqrtPartiallyInlined",
Call->getDebugLoc(), &CurrBB)
<< "Partially inlined call to sqrt function despite having to use "
"errno for error handling: target has fast sqrt instruction";
Copy link
Contributor

@arsenm arsenm May 5, 2025

Choose a reason for hiding this comment

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

This doesn't know if the target has a "fast sqrt instruction" and this shouldn't be considering it

Copy link
Contributor Author

@TiborGY TiborGY May 18, 2025

Choose a reason for hiding this comment

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

But doesn't optimizeSQRT only get called if TTI->haveFastSqrt(Call->getType()) == true?
This is its only call site:

        if (TTI->haveFastSqrt(Call->getType()) &&
            optimizeSQRT(Call, CalledFunc, *CurrBB, BB, TTI,
                         DTU ? &*DTU : nullptr, ORE))
          break;

If the target does not have fast sqrt, the && gets short-circuited to false and optimizeSQRT is never called.
Therefore optimizeSQRT implicitly knows the HW sqrt is fast, in the context of the current codebase.
But you are right, this is quite brittle, so I have moved the remark after the condition that may call optimizeSQRT.

FWIW, I have taken the "fast sqrt instruction" language from here:
https://github.com/llvm/llvm-project/blob/9c60431b673c478606e63ff1e47860eb4eb6af09/llvm/include/llvm/Analysis/TargetTransformInfo.h#L1048C1-L1049C37

});
ORE->emit([&]() {
return OptimizationRemarkMissed(DEBUG_TYPE, "BranchInserted",
Call->getDebugLoc(), &CurrBB)
<< "Branch to library sqrt fn had to be inserted to satisfy the "
"current target's requirement for math functions to set errno on "
Copy link
Contributor

@arsenm arsenm May 5, 2025

Choose a reason for hiding this comment

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

This isn't a property of the target. I also don't see why these two remarks are emitted under the same exact conditions

Copy link
Contributor Author

@TiborGY TiborGY May 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, target may not be the best word for it indeed, but the docs are using a similar language:

On some targets, math library functions never set errno, and so -fno-math-errno is the default. This includes most BSD-derived systems, including Darwin.

I guess I could say target triplet instead of target?

I have created two remarks so I can split them into an optimization passed/missed pair. In my mental model, passed means "LLVM did some profitable transformation" and missed means "something is inhibiting an optimization that could have been profitable" or "LLVM thinks something is not profitable". In this case, applying the partial inlining with the branch is simultaneously both: profitable but could have been better if setting errno wasn't a requirement.
Separating the two aspects enables the user to filter the remark output and e.g. look at only the missed optimization remarks. I often use Compiler Explorer to look at what LLVM says about a piece of code, and I find the ability to filter by remark type useful.

"invalid inputs";
Copy link
Contributor

@fhahn fhahn Feb 14, 2025

Choose a reason for hiding this comment

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

Not sure if the OptimizationRemarkMissed is the best category for this. Maybe include it in the remark above, as it isn't really a missed optimization BranchInserted only happens ifSqrtPartiallyInlined also happens ?

Or maybe just use OptimizationRemark

Copy link
Contributor Author

@TiborGY TiborGY Feb 14, 2025

Choose a reason for hiding this comment

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

Hmm, I can see how this could be a matter of interpretation. I have felt the need to separate the two messages as they convey two different aspects of the same event.
First, the OptimizationRemark signals that a transformation which is expected to be profitable has been done.
Second, the OptimizationRemarkMissed signals that that there is a condition inhibiting this optimization from being even better.
Splitting these enables users to have more relevant information with filtering, eg. if they are only interested in spots where LLVM is struggling to optimize something, they can look at OptimizationRemarkMissed only.
Combining the two messages or turning the second one into a second OptimizationRemark would defeat this.

Perhaps my mental model is wrong, but to me OptimizationRemark means "LLVM did something it believes to be useful" and OptimizationRemarkMissed means "LLVM cannot or will not do something that (hypothetically) could have been useful". Hence the separation.

Copy link
Contributor Author

@TiborGY TiborGY Mar 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If my explanation is satisfactory, and there are no further concerns to be discussed, may I request a merge?
I do not have write access to the LLVM repo.

});

Type *Ty = Call->getType();
IRBuilder<> Builder(Call->getNextNode());

Expand Down Expand Up @@ -125,11 +139,29 @@ static bool runPartiallyInlineLibCalls(Function &F, TargetLibraryInfo *TLI,
if (!Call || !(CalledFunc = Call->getCalledFunction()))
continue;

if (Call->isNoBuiltin() || Call->isStrictFP())
if (Call->isNoBuiltin())
continue;

if (Call->isMustTailCall())
if (Call->isStrictFP()) {
ORE->emit([&]() {
return OptimizationRemarkMissed(DEBUG_TYPE, "StrictFloat",
Call->getDebugLoc(), &*CurrBB)
<< "Could not consider library function for partial inlining:"
" strict FP exception behavior is active";
});
continue;
}
// Partially inlining a libcall that has the musttail attribute leads to
// broken LLVM IR, triggering an assertion in the IR verifier.
// Work around that by forgoing this optimization for musttail calls.
if (Call->isMustTailCall()) {
ORE->emit([&]() {
return OptimizationRemarkMissed(DEBUG_TYPE, "MustTailCall",
Call->getDebugLoc(), &*CurrBB)
<< "Could not consider library function for partial inlining:"
" must tail call";
});
continue;
}

// Skip if function either has local linkage or is not a known library
// function.
Expand Down
6 changes: 5 additions & 1 deletion llvm/test/Transforms/PartiallyInlineLibCalls/X86/good-prototype.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu -pass-remarks=partially-inline-libcalls \
; RUN: -pass-remarks-missed=partially-inline-libcalls 2>%t < %s | FileCheck %s
; RUN: cat %t | FileCheck %s -check-prefix=CHECK-REMARK

define float @f(float %val) {
; CHECK-LABEL: @f(
; CHECK-REMARK: Partially inlined call to sqrt function despite having to use errno for error handling: target has fast sqrt instruction
; CHECK-REMARK: Branch to library sqrt fn had to be inserted to satisfy the current target's requirement for math functions to set errno on invalid inputs
; CHECK-NEXT: entry:
; CHECK-NEXT: [[RES:%.*]] = tail call float @sqrtf(float [[VAL:%.*]]) #[[READNONE:.*]]
; CHECK-NEXT: [[TMP0:%.*]] = fcmp oge float [[VAL]], 0.000000e+00
Expand Down
4 changes: 3 additions & 1 deletion llvm/test/Transforms/PartiallyInlineLibCalls/X86/musttail.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu -pass-remarks-missed=partially-inline-libcalls 2>%t < %s | FileCheck %s
; RUN: cat %t | FileCheck %s -check-prefix=CHECK-REMARK

define double @foo(double %x) {
; CHECK-LABEL: @foo(
; CHECK-REMARK: Could not consider library function for partial inlining: must tail call
; CHECK-NEXT: [[R:%.*]] = musttail call double @sqrt(double [[X:%.*]])
; CHECK-NEXT: ret double [[R]]
;
Expand Down
4 changes: 3 additions & 1 deletion llvm/test/Transforms/PartiallyInlineLibCalls/strictfp.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck %s
; RUN: opt -S -passes=partially-inline-libcalls -mtriple=x86_64-unknown-linux-gnu -pass-remarks-missed=partially-inline-libcalls 2>%t < %s | FileCheck %s
; RUN: cat %t | FileCheck %s -check-prefix=CHECK-REMARK

define float @f(float %val) strictfp {
; CHECK-LABEL: @f
; CHECK-REMARK: Could not consider library function for partial inlining: strict FP exception behavior is active
; CHECK: call{{.*}}@sqrtf
; CHECK-NOT: call{{.*}}@sqrtf
%res = tail call float @sqrtf(float %val) strictfp
Expand Down
Loading