Skip to content
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.

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

libclc: remove __attribute__((assume)) for clspv targets #92126

Merged
merged 1 commit into from
May 17, 2024

Conversation

rjodinchr
Copy link
Contributor

Instead add a proper attribute in clang, and add convert it to function metadata to keep the information in the IR. The goal is to remove the dependency on attribute((assume)) that should have not be there in the first place.

Ref #84934

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:codegen labels May 14, 2024
@llvmbot
Copy link

llvmbot commented May 14, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Romaric Jodin (rjodinchr)

Changes

Instead add a proper attribute in clang, and add convert it to function metadata to keep the information in the IR. The goal is to remove the dependency on attribute((assume)) that should have not be there in the first place.

Ref #84934


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

3 Files Affected:

  • (modified) clang/include/clang/Basic/Attr.td (+6)
  • (modified) clang/lib/CodeGen/CodeGenFunction.cpp (+5)
  • (modified) libclc/generic/include/clc/clcfunc.h (+1-1)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 52552ba488560..0970110ee581d 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4561,3 +4561,9 @@ def CodeAlign: StmtAttr {
     static constexpr int MaximumAlignment = 4096;
   }];
 }
+
+def ClspvLibclcBuiltin: DeclOrStmtAttr {
+  let Spellings = [Clang<"clspv_libclc_builtin">];
+  let Documentation = [Undocumented];
+  let SimpleHandler = 1;
+}
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 9f16fcb438557..4c4f368ddeba1 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -979,6 +979,11 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
     EmitKernelMetadata(FD, Fn);
   }
 
+  if (FD && FD->hasAttr<ClspvLibclcBuiltinAttr>()) {
+    Fn->setMetadata("clspv_libclc_builtin",
+                    llvm::MDNode::get(getLLVMContext(), {}));
+  }
+
   // If we are checking function types, emit a function type signature as
   // prologue data.
   if (FD && SanOpts.has(SanitizerKind::Function)) {
diff --git a/libclc/generic/include/clc/clcfunc.h b/libclc/generic/include/clc/clcfunc.h
index ad9eb23f29333..59f45c27019d6 100644
--- a/libclc/generic/include/clc/clcfunc.h
+++ b/libclc/generic/include/clc/clcfunc.h
@@ -8,7 +8,7 @@
 #define _CLC_DEF
 #elif defined(CLC_CLSPV) || defined(CLC_CLSPV64)
 #define _CLC_DEF                                                               \
-  __attribute__((noinline)) __attribute__((assume("clspv_libclc_builtin")))
+  __attribute__((noinline)) __attribute__((clspv_libclc_builtin))
 #else
 #define _CLC_DEF __attribute__((always_inline))
 #endif

Copy link

github-actions bot commented May 14, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

Also needs:
-a release note
-A test: At least a test against LLVM-IR (CodeGen test).

clang/include/clang/Basic/Attr.td Outdated Show resolved Hide resolved
clang/include/clang/Basic/Attr.td Show resolved Hide resolved
@rjodinchr
Copy link
Contributor Author

@erichkeane, I don't know what you expect for the release note, where should I write something?

@erichkeane
Copy link
Collaborator

@erichkeane, I don't know what you expect for the release note, where should I write something?

See ReleaseNotes.rst, find a section that makes sense (probably attribute changes), and write a quick blurb about the attribute, likely a very condensed version of the docs.

clang/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved
clang/include/clang/Basic/AttrDocs.td Outdated Show resolved Hide resolved
@Sirraide
Copy link
Member

There’s a test that checks all attributes that we support (Misc/pragma-attribute-supported-attributes-list.test) and which is failing at the moment; you’ll have to add this attribute there as well.

Instead add a proper attribute in clang, and add convert it to
function metadata to keep the information in the IR.
The goal is to remove the dependency on __attribute__((assume)) that
should have not be there in the first place.

Ref llvm#84934
Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

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

Still fine once CI approves

@rjodinchr
Copy link
Contributor Author

Is there something else needed on my side to get that PR merged?

@erichkeane erichkeane merged commit 932ca85 into llvm:main May 17, 2024
5 checks passed
@erichkeane
Copy link
Collaborator

Is there something else needed on my side to get that PR merged?

My apologies, I thought you'd been around LLVM enough that you had merge rights. I've done it now.

@rjodinchr rjodinchr deleted the pr/clspv-libclc-builtin branch May 17, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants