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

[clang] add unnamed_addr function attribute #92499

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

Conversation

yamt
Copy link
Contributor

@yamt yamt commented May 17, 2024

It simply applies the LLVM attribute with the same name to the function.

Sometimes, a programmer knows that function pointer uniqueness doesn't really matter for some of their functions. In that case, this attribute opens a possibility of certain optimizations like mergefunc with aliases. It's especially useful for code generators.

@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 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 17, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: YAMAMOTO Takashi (yamt)

Changes

It simply applies the LLVM attribute with the same name to the function.

Sometimes, a programmer knows that function pointer uniqueness doesn't really matter for some of their functions. In that case, this attribute opens a possibility of certain optimizations like mergefunc with aliases. It's especially useful for code generators.


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

2 Files Affected:

  • (modified) clang/include/clang/Basic/Attr.td (+7)
  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+4)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 52552ba488560..3ee7d43d339f1 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1944,6 +1944,13 @@ def ReturnsTwice : InheritableAttr {
   let SimpleHandler = 1;
 }
 
+def UnnamedAddr : InheritableAttr {
+  let Spellings = [Clang<"unnamed_addr">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];
+  let SimpleHandler = 1;
+}
+
 def DisableTailCalls : InheritableAttr {
   let Spellings = [Clang<"disable_tail_calls">];
   let Subjects = SubjectList<[Function, ObjCMethod]>;
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 489c08a4d4819..ac9f082a1049b 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -2506,6 +2506,10 @@ void CodeGenModule::SetLLVMFunctionAttributesForDefinition(const Decl *D,
       B.addAttribute(llvm::Attribute::MinSize);
   }
 
+  if (D->hasAttr<UnnamedAddrAttr>()) {
+    F->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
+  }
+
   F->addFnAttrs(B);
 
   unsigned alignment = D->getMaxAlignment() / Context.getCharWidth();

It simply applies the LLVM attribute with the same name to the function.

Sometimes, a programmer knows that function pointer uniqueness doesn't
really matter for some of their functions. In that case, this attribute
opens a possibility of certain optimizations like mergefunc with aliases.
It's especially useful for code generators.
@yamt yamt changed the title clang: add unnamed_addr function attribute [clang] add unnamed_addr function attribute May 17, 2024
Copy link
Contributor

@Sirraide Sirraide left a comment

Choose a reason for hiding this comment

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

I’m not sure how useful such an attribute would be, but the implementation of this looks ok, though this needs some documentation, a release note, and some sema tests to make sure we diagnose it when it’s applied to a non-function and codegen tests as well to make sure we actually emit it.

def UnnamedAddr : InheritableAttr {
let Spellings = [Clang<"unnamed_addr">];
let Subjects = SubjectList<[Function]>;
let Documentation = [Undocumented];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for new attributes, we generally do prefer to document them. This attribute especially seems like it would benefit from documentation, so please add some for it. Doesn’t have to be much, just a sentence or two explaining that it adds the LLVM attribute and an example would be enough imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i added documentation

@Sirraide Sirraide requested a review from erichkeane May 17, 2024 15:57
@efriedma-quic
Copy link
Collaborator

If we're going to do this, it should probably also work for constants.

Also, I think I'd prefer to sort out the situation with the C++ standard's rules for constant merging before we start extending those rules. See #63628.

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.

Hmm... I'm not sure this meets our requirements for inclusion as an attribute. The semantics of this are pretty opaque, no obvious significant motivation/applicability in the base languages, etc. There doesn't seem to be any reasonable use case that I can see.

The attribute itself in LLVM isn't sufficient motivation for inclusion of this attribute.

@yamt
Copy link
Contributor Author

yamt commented May 21, 2024

Hmm... I'm not sure this meets our requirements for inclusion as an attribute. The semantics of this are pretty opaque, no obvious significant motivation/applicability in the base languages, etc. There doesn't seem to be any reasonable use case that I can see.

do you mean my use case is not reasonable?
or it isn't well explained?
or something else?

@yamt
Copy link
Contributor Author

yamt commented May 21, 2024

If we're going to do this, it should probably also work for constants.

for completeness, maybe. i myself have no use cases though.

Also, I think I'd prefer to sort out the situation with the C++ standard's rules for constant merging before we start extending those rules. See #63628.

interesting. i was not aware of it.

@erichkeane
Copy link
Collaborator

Hmm... I'm not sure this meets our requirements for inclusion as an attribute. The semantics of this are pretty opaque, no obvious significant motivation/applicability in the base languages, etc. There doesn't seem to be any reasonable use case that I can see.

do you mean my use case is not reasonable? or it isn't well explained? or something else?

This could be either. At the moment, I don't see the use of this to be compelling enough to be in the compiler. That said, the description of the use case is ~3 short sentences. In general, implementing LLVM attributes in Clang directly is typically not sufficient.

Also, I agree with Eli, IF we do decide to do something like this, it needs to be holistically integrated/designed with the source language.

@efriedma-quic
Copy link
Collaborator

I think the underlying functionality is pretty clearly useful: identical code folding gives significant codesize reductions. In fact, on Windows, the linker does this kind of folding automatically by default (despite the fact that it isn't standards-compliant). I imagine if we had this implemented, libc++ would want to use it.

@yamt
Copy link
Contributor Author

yamt commented May 22, 2024

That said, the description of the use case is ~3 short sentences.

ok.
here is a concrete example and a bit longer explanation:
https://github.com/yamt/toywasm/blob/9ee6ec86f56723819fd8411866094f72247dba78/lib/insn.c#L515-L527

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.

None yet

5 participants