This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add a flag for embedding a file into the module
ClosedPublic

Authored by jhuber6 on Jan 3 2022, 9:55 AM.

Details

Summary

This patch adds support for a flag -fembed-offload-binary to embed a
file as an ELF section in the output by placing it in a global variable.
This can be used to bundle offloading files with the host binary so it
can be accessed by the linker. The section is named using the
-fembed-offload-section option.

Depends on D116541

Diff Detail

Event Timeline

jhuber6 created this revision.Jan 3 2022, 9:55 AM
jhuber6 requested review of this revision.Jan 3 2022, 9:55 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 3 2022, 9:55 AM
jhuber6 updated this revision to Diff 397590.Jan 5 2022, 8:24 AM

Clang format

ormris removed a subscriber: ormris.Jan 18 2022, 10:16 AM
JonChesterfield requested changes to this revision.Jan 26 2022, 10:10 AM

I don't think this works for multiple files. The test only tries one and misses all the parsing handling.

clang/include/clang/Basic/CodeGenOptions.h
279

This is unclear. List in what sense? It's a std::string, which suggests it's a freeform argument that gets parsed somewhere else.

The relation between the two strings is also unclear. Are the lists meant to be the same length, implying a one to one mapping? If there is a strong relation between the two we can probably remove a bunch of error handling by taking one argument instead of two.

Perhaps the variable should be something like a vector<pair<filename, section>>?

clang/lib/CodeGen/BackendUtil.cpp
1764

Definitely don't want to assert on invalid commandline argument

1781

This looks lossy - if two files use the same section name, they'll end up appended in an order that is probably an implementation quirk of llvm-link, and I think we've thrown away the filename info so can't get back to where we were.

Would .llvm.offloading.filename be a reasonable name for each section, with either error on duplicates or warning + discard?

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4984

I think I've written some handling very like this in an LDS pass that I meant to factor out for reuse but haven't got around to - we're removing a value from compiler.used?

4985

This removes all variables with that prefix

4996

missing end of comment

5005

Is this necessary? 1 seems a likely default for a uint8_t array

5009

And this pushes one global back into that array

So this looks like it'll mark the last embedded file as .used and none of the others

This revision now requires changes to proceed.Jan 26 2022, 10:10 AM
llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4893

This seems to be where the code was copied from. Looks broken here too, unless there's some constraint which means exactly one things is embedded at a time - which might be the case given the .module suffix

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4936

here's an assert that this function only handled one at a time

4970

This is probably better written in terms of a call to a function like removeFromUsedList, which exists in AMDGPULowerModuleLDSPass at present because I forgot to move it and failed to find an existing implementation. Perhaps something that should be next to llvm::collectUsedGlobalVariables in Module.cpp?

jhuber6 added inline comments.Jan 26 2022, 10:20 AM
clang/include/clang/Basic/CodeGenOptions.h
279

It's a list of comma separated values, but you're right. This should be parsed out when we handle the flags.

clang/lib/CodeGen/BackendUtil.cpp
1764

Will remove.

1781

We only care about the sections per-file right. When I extract these in the linker-wrapper I simply look at each file's sections, and put them into a list of device inputs, we don't need them to be unique as long as there aren't multiple in the same file.

llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
4984

This handling of the compiler.used variable was copied from the implementation of -fembed-bitcode above. I wasn't sure about their methodology but I figured it worked for them. I can tidy it up to be more straightforward.

4996

Will fix.

5005

Will remove.

jhuber6 updated this revision to Diff 403404.Jan 26 2022, 2:18 PM

Updating approach, use a vector of string pairs now. Multiple files are simply
passed multiple times. Will add filename to the offloading section name laterf,
as similar sections could be merged when performing a relocatable link.

jhuber6 added inline comments.Jan 26 2022, 2:19 PM
llvm/lib/Bitcode/Writer/CMakeLists.txt
14

I'm not sure if it's worth linking TransformUtils just for appendToCompilerUsed I can copy the implementation locally if not.

jhuber6 updated this revision to Diff 403468.Jan 26 2022, 7:08 PM

clang format.

jhuber6 updated this revision to Diff 403476.Jan 26 2022, 7:49 PM

Forgot to rename file.

This might be OK. If multiple translation units still get fed to llvm-link it'll be broken, which might be what we get on amdgpu at present.

Not totally clear to me whether this should be compiler.used or .used, as it's used by something after clang but before the linker. However I recently learned that compiler.used and used are the same thing on elf anyway, so it doesn't matter hugely. Going with compiler.used for now and then backing it off to .used later if something starts deleting them seems fine.

clang/lib/CodeGen/BackendUtil.cpp
1781

I think we'll have problems if multiple files are embedded with the same section string, as they'll get concatenated in the output. llvm-link or ld -r on the host bitcode files will hit that.

It would be worth testing this with two input files, for the same offloading architecture, on amdgpu since I think it will feed the host bitcode to llvm-link which will implicitly concatenate the two embedded files.

clang/test/Frontend/embed-object.ll
3

I think we need a test case with more than one embedded file, given there's the careful splitting around commas in the implementation

llvm/lib/Bitcode/Writer/CMakeLists.txt
14

Definitely better to link in an LLVM library than to copy&paste functions out of it

jhuber6 added inline comments.Jan 28 2022, 7:53 AM
clang/lib/CodeGen/BackendUtil.cpp
1781

This scheme works on AMDGPU because we don't use llvm-link as a part of the driver anymore, the new scheme unifies the behavior between NVPTX and AMDGPU until we hit the linker wrapper. But you're right that if the user creates host bitcode and runs llvm-link on that, or performs a relocatable link, we'll get conflicts. I can add a unique string at the end of the section name to avoid this in a later patch.

clang/test/Frontend/embed-object.ll
3

I only split to create a pair now, I can make a test where we pass this flag multiple times.

jhuber6 updated this revision to Diff 404037.Jan 28 2022, 8:22 AM

Adding test for multiple files (added it to wrong commit).

jhuber6 updated this revision to Diff 404047.Jan 28 2022, 8:58 AM

Changing the name to be the section name. This ensures that if the sections get merged we will get a linker error without failing silently.

clang/lib/CodeGen/BackendUtil.cpp
1760

Could we have a type here instead of auto? I'm trying to guess what a StringRef split might return and there seems to be a few choices. If it's a container of some sort we could error on size() != 2

clang/lib/CodeGen/BackendUtil.cpp
1771

OK, so on failure to parse (missing comma?), this will be an empty string, and the section created will be .llvm.offloading.. Given that sections with the same name are implicitly concatenated that's a sharp edge, can we fatal error on the second field being empty? It indicate a badly formed commandline argument of some sort

clang/lib/CodeGen/BackendUtil.cpp
1760

Nvm, it's a pair.

We could add

if (OffloadObject.count(',') !=1 )  {
    Diags.Report(DiagID) << OffloadObject;
   return;
}
jhuber6 updated this revision to Diff 404506.Jan 31 2022, 6:54 AM

Add error handling routine to ensure that the embedding string is always a pair separated by a single ','.

JonChesterfield accepted this revision.Jan 31 2022, 6:57 AM

Thanks for sticking with me on this! LG

This revision is now accepted and ready to land.Jan 31 2022, 6:57 AM
This revision was landed with ongoing or failed builds.Jan 31 2022, 12:56 PM
This revision was automatically updated to reflect the committed changes.
cmtice added a subscriber: cmtice.Jan 31 2022, 3:35 PM

This change introduces a circular dependency: BitcodeWriters now depends on TransformUtils, but TransformUtils also depends on BitcodeWriters. This appears to be a layering violation.

This change introduces a circular dependency: BitcodeWriters now depends on TransformUtils, but TransformUtils also depends on BitcodeWriters. This appears to be a layering violation.

Might explain why it wasn't included before, should I just copy the function I here and remove the dependency?

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

That's fine, I put it here originally because it was grouped with another similar function. But it's likely that one should be moved as well.

@jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9.. OK, I was late.

See D118666 for the proper fix.

I'd be better to revert this relevant changes if that doesn't make you feel back.
I can prepare the revert.

That's fine, I put it here originally because it was grouped with another similar function. But it's likely that one should be moved as well.

Thanks:) The issue should be resolved by 85dfe19b36ba6e9657612e072c9183ce168fdbbc. No need to revert.