This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Allow bitcode linking when the input is LLVM-IR
ClosedPublic

Authored by jhuber6 on Jun 7 2023, 11:16 AM.

Details

Summary

Clang provides the -mlink-bitcode-file and -mlink-builtin-bitcode
options to insert LLVM-IR into the current TU. These are usefuly
primarily for including LLVM-IR files that require special handling to
be correct and cannot be linked normally, such as GPU vendor libraries
like libdevice.10.bc. Currently these options can only be used if the
source input goes through the AST consumer path. This patch makes the
changes necessary to also support this when the input is LLVM-IR. This
will allow the following operation:

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

Diff Detail

Event Timeline

jhuber6 created this revision.Jun 7 2023, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 11:16 AM
jhuber6 requested review of this revision.Jun 7 2023, 11:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2023, 11:16 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

That's cleaner than I expected, thanks. Might be reasonable to factor out the method as an initial NFC then insert the call to it along with the new test case as the functional change.

tra added a comment.Jun 7 2023, 12:06 PM

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

If that's something we intend to expose to the user, should we consider promoting it to a top-level driver option?

clang in.bc -Xclang -mlink-builtin-bitcode -Xclang libdevice.10.bc

If that's something we intend to expose to the user, should we consider promoting it to a top-level driver option?

I'm not sure, this probably wouldn't be relevant for non-compiler developers so I'm fine keeping it as an -Xclang workaround.

Just realized this is probably going to be a bit more painful. the attribute propagation pass requires a CodeGenModule which isn't built without an ASTContext so it's not available here. Nothing those functions do explicitly requires the full CGM, they only use the options and the target info. So it's possible to make these functions more generic using only the CompilerInstance instead, but it'll be a lot nosier.

jhuber6 added a comment.EditedJun 7 2023, 3:59 PM

Scratch that, ASTContext::getFunctionFeatureMap is used. I wonder if we could forgo that.

Actually, that's only used if there's a GlobalDecl which we don't have when we call this. So that could be ignored here.

jhuber6 updated this revision to Diff 529490.Jun 7 2023, 8:06 PM

Updating, in order to do this starting with bitcode I had to expose a helper
that performs this operation using the options directly rather than through the
CodeGenModule. This should keep the existing interfaces intact by shuttling
them through a new internal function while also letting me write this standalone
function that can be used without a CodeGenModule.

jhuber6 updated this revision to Diff 529557.Jun 8 2023, 4:43 AM

Add a better test to show that attributes are replaced and default attributes are added.

yaxunl added inline comments.Jun 13 2023, 8:08 AM
clang/lib/CodeGen/CGCall.cpp
2057–2111

can we reorder these functions to minimize the diffs? Also some comments about the difference among these functions may help.

clang/lib/CodeGen/CodeGenAction.cpp
266

Removing the default argument may make the code clearer

jhuber6 updated this revision to Diff 530919.Jun 13 2023, 8:19 AM
jhuber6 marked 2 inline comments as done.

Addressing comments.

jhuber6 marked an inline comment as not done.Jun 13 2023, 8:20 AM
jhuber6 added inline comments.
clang/lib/CodeGen/CGCall.cpp
2057–2111

I added some comments. These can't be reordered because they're new functions, I simply added a layer of indirection to go through a common helper so we can call the main bulk of the code without needing the full CGM.

yaxunl added inline comments.Jun 13 2023, 9:19 AM
clang/lib/CodeGen/CGCall.cpp
2107–2108

should this be removed? seems redundant

jhuber6 marked an inline comment as not done.Jun 13 2023, 9:21 AM
jhuber6 added inline comments.
clang/lib/CodeGen/CGCall.cpp
2107–2108

I wasn't entirely sure, there's a lot of work done in that function but pretty much all of it goes away since we just use GlobalDecl(). So for the version that doesn't have access to CGM I copies out the only two relevant lines and put them in line 2063. We could probably make this use the same code but I wasn't sure if there was something else we might need it for.

yaxunl added inline comments.Jun 13 2023, 9:38 AM
clang/lib/CodeGen/CGCall.cpp
2107–2108

I tend to think they are equivalent. If GetCPUAndFeaturesAttributes did something else, it may be overwritten by the counterparts in mergeDefaultFunctionDefinitionAttributes.

clang/lib/CodeGen/CGCall.cpp
1994–2000

I'm used to this sort of copy-some-args-and-not-others showing up in bug reports. Could this patch be re-ordered to make it apparent what functional changes are happening relative to the current head?

jhuber6 added inline comments.Jun 13 2023, 12:31 PM
clang/lib/CodeGen/CGCall.cpp
1994–2000

It's the same that will get added in in the GetCPUAndFeaturesAttributes function if you pass in a null declaration.

yaxunl accepted this revision.Jun 19 2023, 8:14 PM

LGTM. Thanks.

This revision is now accepted and ready to land.Jun 19 2023, 8:14 PM
This revision was automatically updated to reflect the committed changes.

LGTM. CC1 -mlink-bitcode-file is an interesting option from 2011: f1d76db466b2a50781c0754b86ac994dd07b5041
I wonder what the original use case is...