Skip to content

[HLSL] Appropriately set function attribute optnone #125937

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 4 commits into from
Feb 11, 2025
Merged

[HLSL] Appropriately set function attribute optnone #125937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63a728d
[HLSL] Set function optnone attribute appropriately
bharadwajy Feb 4, 2025
00897b8
[HLSL] Delete the change that sets non-entry functions of a library
bharadwajy Feb 7, 2025
54b1e87
Add comment
bharadwajy Feb 11, 2025
37bcc29
Delete braces around simple single-statement body of if statement
bharadwajy Feb 11, 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
7 changes: 7 additions & 0 deletions clang/lib/CodeGen/CGHLSLRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,13 @@ void clang::CodeGen::CGHLSLRuntime::setHLSLEntryAttributes(
WaveSizeAttr->getPreferred());
Fn->addFnAttr(WaveSizeKindStr, WaveSizeStr);
}
// HLSL entry functions are materialized for module functions with
// HLSLShaderAttr attribute. SetLLVMFunctionAttributesForDefinition called
// later in the compiler-flow for such module functions is not aware of and
// hence not able to set attributes of the newly materialized entry functions.
// So, set attributes of entry function here, as appropriate.
if (CGM.getCodeGenOpts().OptimizationLevel == 0)
Fn->addFnAttr(llvm::Attribute::OptimizeNone);
Fn->addFnAttr(llvm::Attribute::NoInline);
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that these already have "noinline", I'm surprised that the logic in "SetLLVMFunctionAttributesForDefinition" doesn't already put optnone on these functions. Is something undoing this later?

Copy link
Contributor Author

@bharadwajy bharadwajy Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Given that these already have "noinline", I'm surprised that the logic in "SetLLVMFunctionAttributesForDefinition" doesn't already put optnone on these functions. Is something undoing this later?

The entry function that is created and whose attribute is set to noinline in this function is different from that SetLLVMFunctionAttributesForDefinition() looks at.

GenerateCode(GlobalFnDecl, MangledFn, ...) calls StartFunction(GlobalFnDecl, ResTy, MangledFn, ...) which in turn calls emitEntryFunction(FnDecl, MangledFn). emitEntryFunction(FnDecl, MangledFn, ...) constructs a new entry function EntryFn, sets linkage of MangledFn to be internal to arrange it to be inlined in EntryFn etc., and calls setHLSLEntryAttributes(FnDecl, EntryFn) to set attributes of EntryFn.

SetLLVMFunctionAttributesForDefinition(...) sets attributes of MangledFn. So the logic in that function checks attributes for MangledFn and not for the created EntryFn.

Hence setting optnone attribute in setHLSLEntryAttributes(FnDecl, EntryFn) of EntryFn at the time of its set up seemed appropriate - if optimizations are disabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I think a comment to the effect of "We need to manually set attributes here instead of relying on SetLLVMFunctionAttributesForDefinition to pick them up since these functions are injected by the compiler and won't go through the normal flow" (please reword as necessary to be accurate...) would be a good idea here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I think a comment to the effect of "We need to manually set attributes here instead of relying on SetLLVMFunctionAttributesForDefinition to pick them up since these functions are injected by the compiler and won't go through the normal flow" (please reword as necessary to be accurate...) would be a good idea here.

Comment added. Thanks!

}

Expand Down
16 changes: 9 additions & 7 deletions clang/test/CodeGenHLSL/inline-functions.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -disable-llvm-passes -o - | FileCheck %s --check-prefixes=CHECK,NOINLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -O0 -o - | FileCheck %s --check-prefixes=CHECK,INLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -O1 -o - | FileCheck %s --check-prefixes=CHECK,INLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -disable-llvm-passes -o - | FileCheck %s --check-prefixes=CHECK,NOINLINE,OPT_ATTR
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -O0 -o - | FileCheck %s --check-prefixes=CHECK,INLINE,OPT_ATTR
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.3-library %s -emit-llvm -O1 -o - | FileCheck %s --check-prefixes=CHECK,INLINE,NOOPT_ATTR
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute %s -emit-llvm -disable-llvm-passes -o - | FileCheck %s --check-prefixes=CHECK,NOINLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute %s -emit-llvm -O0 -o - | FileCheck %s --check-prefixes=CHECK,INLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute %s -emit-llvm -O1 -o - | FileCheck %s --check-prefixes=CHECK,INLINE
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute %s -emit-llvm -O0 -o - | FileCheck %s --check-prefixes=CHECK,INLINE,OPT_ATTR
// RUN: %clang_cc1 -triple dxil-pc-shadermodel6.0-compute %s -emit-llvm -O1 -o - | FileCheck %s --check-prefixes=CHECK,INLINE,NOOPT_ATTR

// Tests that user functions will always be inlined.
// This includes exported functions and mangled entry point implementation functions.
Expand Down Expand Up @@ -71,7 +71,8 @@ RWBuffer<unsigned> Indices;
// NOINLINE: ret void

// The unmangled version is not inlined, EntryAttr reflects that
// CHECK: Function Attrs: {{.*}}noinline
// OPT_ATTR: Function Attrs: {{.*}}optnone
// NOOPT_ATTR-NOT: Function Attrs: {{.*}}optnone
// CHECK: define void @main() {{[a-z_ ]*}}[[EntryAttr:\#[0-9]+]]
// Make sure function calls are inlined when AlwaysInline is run
// This only leaves calls to llvm. intrinsics
Expand All @@ -98,7 +99,8 @@ void main(unsigned int GI : SV_GroupIndex) {
// NOINLINE: ret void

// The unmangled version is not inlined, EntryAttr reflects that
// CHECK: Function Attrs: {{.*}}noinline
// OPT_ATTR: Function Attrs: {{.*}}optnone
// NOOPT_ATTR-NOT: Function Attrs: {{.*}}optnone
// CHECK: define void @main10() {{[a-z_ ]*}}[[EntryAttr]]
// Make sure function calls are inlined when AlwaysInline is run
// This only leaves calls to llvm. intrinsics
Expand Down
Loading