Page MenuHomePhabricator

Add __attribute__((linkonce_odr_linkage))
AbandonedPublic

Authored by yaxunl on Mar 11 2016, 12:04 PM.

Details

Summary

Add attribute((linkonce_odr_linkage)) to allow setting LLVM linkonce_odr linkage explicitly.

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 50460.Mar 11 2016, 12:04 PM
yaxunl retitled this revision from to Add __attribute__((linkonce_odr_linkage)).
yaxunl updated this object.
yaxunl added reviewers: aaron.ballman, rjmccall.
yaxunl set the repository for this revision to rL LLVM.
yaxunl added a subscriber: tstellarAMD.
yaxunl added a subscriber: cfe-commits.
rjmccall edited edge metadata.Mar 12 2016, 11:50 AM

Okay, first off, linkonce_odr is not an acceptable term to be introducing as an attribute name. If we need this, we can find some way to describe it that isn't a random internal compiler term.

More importantly, I don't understand how your problem is solved by linkonce_odr linkage. linkonce_odr linkage is just a form of weak linkage that allow useful transformations by the compiler (specifically: the compiler can freely drop the function if it isn't being used, and it can assume that all the implementations are semantically equivalent). If you make a library with a bunch of architecture-specific linkonce_odr implementations of a function, the linker will still just pick one randomly; it won't allow them to co-exist.

yaxunl added a comment.EditedMar 14 2016, 10:45 AM

Thanks for your comments.

It works like this, e.g.

$ cat prog.ll

declare i32 @foo()

define void @use_foo() {
   %a = call i32 @foo()
  ret void
}

$ cat lib_common.ll

define linkonce_odr i32 @foo() {
  ret i32 1;
}

$ cat lib_opt.ll

define linkonce_odr i32 @foo() {
  ret i32 2;
}

$ llvm-link prog.ll lib_common.ll -override lib_opt.ll -S

; ModuleID = 'llvm-link'

define void @use_foo() {
  %a = call i32 @foo()
  ret void
}

define linkonce_odr i32 @foo() {
  ret i32 2
}

We can put all common functions in lib_common.ll, then only put a subset in lib_opt.ll. Functions in lib_opt.ll will override functions in lib_common.ll. For different GPUs we provide different lib_opt.ll. Each GPU may override different subset of lib_common.ll.

We use attribute((linkonce_odr_linkage)) by following the precedence of attribute((internal_linkage)) which exposes the LLVM internal_linkage to C/C++ programmers. We would like to accept suggestions for a better way to expose the linkonce_odr linkage.

Your use case violates the "ODR" restriction on linkonce_odr.

Do you maybe just want attribute((weak))?

My last example is not proper. In real cases, the functions are overridden by functions with the same name and semantics but optimized for speed.

Besides, we want unused library functions in lib_common.ll and lib_opt.ll to be dropped, weak attribute does not achieve that.

linkonce_odr would allow them to be dropped if unused by the library. In fact, we don't normally emit IR for functions that are linkonce and not used.

Do you actually want code in lib_common to e.g. inline the common implementation instead of calling the optimized one if present? Because that is also allowed by linkonce_odr.

Yes we want the overriding functions to be allowed to be inlined.

Well, your overriding definitions will be strong definitions. The question is whether you want to allow inning of the weak definitions, i.e. the possibly-overridden ones, and there I would assume not.

we can turn off inlining when we build lib_common.ll, then do optimization and inlining after linking with lib_common.ll and lib_opt.ll. Even if linkonce_odr allows inlining, it is still OK.

yaxunl updated this revision to Diff 51297.Mar 22 2016, 10:26 AM
yaxunl updated this object.
yaxunl edited edge metadata.
yaxunl removed rL LLVM as the repository for this revision.

Simplify description of this attribute in AttrDocs since it causes some confusion.

If attribute((linkonce_odr_linkage)) is not a proper name for explicitly setting linkage, how about __attribute((linkage=linkonce_odr))? This can be extended to other linkages too.

You still don't actually want linkonce_odr linkage. You don't want the weak definition to be inlined, so it can't be ODR, and you want to force it to be emitted in your library, so it can't be linkonce. You just want weak linkage. There's an existing attribute for that.

At best, you want a pass on your completed library to promote any still-weak definitions to strong. That is not something we can usefully support in an attribute; it needs a custom pass.

Dropping unused definitions from your library when it's linked into an actual application is just standard dead-code stripping and does not need a special linkage.

Sorry my previous example may have caused some confusion.

Previously I said I wanted to override function definitions. However the only reason we want to add this attribute is so that unused functions will be dropped by the linker.

Does your linker not supported dead-code stripping?

You could also get this effect by somehow making the definitions linkonce_odr when they're linked in from the library. Or you could simply use the classic static-archive technique of putting each symbol in its own object file and linking against the whole thing as a static archive (.a), which only pulls in object files that provide symbols that are used.

Regardless, linkonce won't get the effect you want, because (1) linkonce definitions can be dropped at any time when they're not used, so they won't be emitted in the library if they're not used there, and (2) the overriding definitions will be strong.

Hi John,

The problem we are trying to solve here is linking a LLVM bitcode program (OpenCL kernel in this specific case) with an LLVM bitcode library (OpenCL builtin library) and having the resulting LLVM bitcode module contain only the program code and the library functions that it uses.

The solution for this problem that we are using for libclc is to compile the OpenCL library to bitcode and then run a post-processing pass: https://github.com/llvm-mirror/libclc/blob/master/utils/prepare-builtins.cpp on the bitcode to set the linkage of the library functions to linkonce_odr. Doing this gives us what we are looking for, because the LLVM IR linker will drop all the linkonce_odr definitions that aren't used when it links the bitcode library in with the program.

The rationale for this specific patch was that if we could apply the linkonce_odr attribute to the functions at the source level, then we could avoid having to use the LLVM IR post-processing pass, which seems like a much cleaner and more portable solution.

Based on this comment:

You could also get this effect by somehow making the definitions linkonce_odr when they're linked in from the library.

It seems like you are suggesting that a post-processing pass like we have would be better, but I'm not sure if you had the complete picture of what we were trying to do. GIven the problem I've described above is a post-processing pass the way to go or do you have some other suggestion?

Thanks,
Tom

Yes, you should just stick with your post-processing pass or something like it.

The design of linkonce_odr linkage is that such definitions will only be emitted when they are actually used. Even with this attribute, a translation unit that consists solely of:

__attribute__((linkonce_odr_linkage)) void foo() {}

would naturally be emitted as an empty object file. It is possible that Clang as patched will not do this, because you haven't updated the places that implement lazy code emission to be aware of the new attribute. However, that strikes me as a bug, not a guaranteed aspect of the semantics of the attribute. Furthermore, LLVM module-level optimizations could also theoretically drop the function body at any time as soon as they see that it is unused.

It seems to me that the right solution is for the definitions to remain strong and then only be selectively brought in when necessary to satisfy dependencies. That could be done with a post-pass that makes the definitions linkonce_odr, or perhaps with an IR linker that simulates static-archive behavior.

yaxunl abandoned this revision.Apr 12 2016, 12:28 PM