Skip to content

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

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

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 IR generation bugs: mangling, exceptions, etc. labels May 14, 2024
@llvmbot
Copy link
Member

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.

@rjodinchr rjodinchr force-pushed the pr/clspv-libclc-builtin branch from 4dc2646 to 41e2a5c Compare May 14, 2024 14:14
@erichkeane erichkeane requested a review from Sirraide May 14, 2024 14:17
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).

@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.

@rjodinchr rjodinchr force-pushed the pr/clspv-libclc-builtin branch from 41e2a5c to 05fd8c2 Compare May 14, 2024 15:11
@rjodinchr rjodinchr requested a review from erichkeane May 14, 2024 15:11
@rjodinchr rjodinchr force-pushed the pr/clspv-libclc-builtin branch from 05fd8c2 to df79338 Compare May 14, 2024 15:31
@rjodinchr rjodinchr requested a review from erichkeane May 14, 2024 15:32
@rjodinchr rjodinchr force-pushed the pr/clspv-libclc-builtin branch from df79338 to 4ce295d Compare May 14, 2024 15:40
@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
@rjodinchr rjodinchr force-pushed the pr/clspv-libclc-builtin branch from 4ce295d to c879883 Compare May 14, 2024 17:04
@rjodinchr rjodinchr requested a review from erichkeane May 15, 2024 07:31
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 IR generation bugs: mangling, exceptions, etc. 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