Discussion:
[PATCH] D18095: Add __attribute__((linkonce_odr_linkage))
(too old to reply)
John McCall via cfe-commits
2016-03-12 19:50:49 UTC
Permalink
rjmccall added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-14 17:45:40 UTC
Permalink
yaxunl added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Richard Smith via cfe-commits
2016-03-14 18:17:27 UTC
Permalink
On Mon, Mar 14, 2016 at 10:45 AM, Yaxun Liu via cfe-commits
Post by Yaxun Liu via cfe-commits
yaxunl added a comment.
Thanks for your comments.
It works like this, e.g.
$ cat prog.ll
ret void
}
$ cat lib_common.ll
ret i32 1;
}
$ cat lib_opt.ll
ret i32 2;
}
$ llvm-link prog.ll lib_common.ll -override lib_opt.ll -S
; ModuleID = 'llvm-link'
ret void
}
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.
The above is not an appropriate use for linkonce_odr linkage, because
the different definitions do not have the same semantics. It sounds
like you just want a weak symbol, which you can already get with
__attribute__((weak)).
John McCall via cfe-commits
2016-03-14 18:17:26 UTC
Permalink
rjmccall added a comment.

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

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


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-14 18:32:03 UTC
Permalink
yaxunl added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-14 18:58:06 UTC
Permalink
rjmccall added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-14 19:07:32 UTC
Permalink
yaxunl added a comment.

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


Repository:
rL LLVM

http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-14 19:18:50 UTC
Permalink
rjmccall added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-14 19:38:55 UTC
Permalink
yaxunl added a comment.

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.


Repository:
rL LLVM

http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-22 17:26:06 UTC
Permalink
yaxunl updated the summary for this revision.
yaxunl removed rL LLVM as the repository for this revision.
yaxunl updated this revision to Diff 51297.
yaxunl added a comment.

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


http://reviews.llvm.org/D18095

Files:
include/clang/Basic/Attr.td
include/clang/Basic/AttrDocs.td
include/clang/Sema/Sema.h
lib/CodeGen/CodeGenModule.cpp
lib/Sema/SemaDeclAttr.cpp
test/CodeGen/attr-linkonce-odr-linkage.c
test/Sema/attr-linkonce-odr-linkage.c
Yaxun Liu via cfe-commits
2016-03-22 17:30:24 UTC
Permalink
yaxunl added a comment.

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.


http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-22 19:58:53 UTC
Permalink
rjmccall added a comment.

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.


http://reviews.llvm.org/D18095
Yaxun Liu via cfe-commits
2016-03-22 20:31:12 UTC
Permalink
yaxunl added a comment.

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.


http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-22 20:32:20 UTC
Permalink
rjmccall added a comment.

Does your linker not supported dead-code stripping?


http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-22 20:36:37 UTC
Permalink
rjmccall added a comment.

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.


http://reviews.llvm.org/D18095
Tom Stellard via cfe-commits
2016-03-22 20:56:06 UTC
Permalink
tstellarAMD added a comment.

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.
Post by John McCall via cfe-commits
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


http://reviews.llvm.org/D18095
John McCall via cfe-commits
2016-03-22 21:19:38 UTC
Permalink
rjmccall added a comment.

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.


http://reviews.llvm.org/D18095

Loading...