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

[jhuber6]

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

[jhuber6]

Clang format

[JonChesterfield]

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

[JonChesterfield]

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

[JonChesterfield]

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]

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]

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]

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]

clang format.

[jhuber6]

Forgot to rename file.

[JonChesterfield]

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]

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]

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

[jhuber6]

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.

[JonChesterfield]

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

[JonChesterfield]

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

[JonChesterfield]

clang/lib/CodeGen/BackendUtil.cpp

1760

Nvm, it's a pair.

We could add

if (OffloadObject.count(',') !=1 )  {
    Diags.Report(DiagID) << OffloadObject;
   return;
}

[jhuber6]

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

[JonChesterfield]

Thanks for sticking with me on this! LG

[cmtice]

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

[jhuber6]

In D116542#3285857, @cmtice wrote:

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?

[MaskRay]

@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]

In D116542#3285983, @MaskRay wrote:

@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.

[MaskRay]

In D116542#3285985, @jhuber6 wrote:

In D116542#3285983, @MaskRay wrote:

@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.