Currently we use the .llvm.offloading section to store device-side
objects inside the host, creating a fat binary. The contents of these
sections is currently determined by the name of the section while it
should ideally be determined by its type. This patch adds the new
SHT_LLVM_OFFLOADING section type to the ELF section types. Which
should make it easier to identify this specific data format.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
220 ms | x64 debian > BOLT.runtime/X86::user-func-reorder.c |
Event Timeline
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp | ||
---|---|---|
504–506 | I'm not sure about just checking the name here. The way these are generated by clang is just dumping the contents of a file to a constant string global in LLVM-IR like so: @llvm.embedded.object = private constant [0 x i8] zeroinitializer, section ".llvm.offloading", align 8 I could probably add some metadata node to indicate this instead like this, but I wasn't sure if it was worth introducing yet more attributes for something that's somewhat niche like this. @llvm.embedded.object = private constant [0 x i8] zeroinitializer, section ".llvm.offloading", align 8 !offloading Also, I may want to distinguish between "offloading" data intended for linking or execution. The former should get the SHF_EXCLUDE flag while the latter should not. It would be nice to be able to pass this to the backend somehow. Let me know what you think. |
We probably need a test case showing that the assembler understands "@llvm_offloading" when reading it (and produces an SHT_LLVM_OFFLOADING section).
llvm/docs/Extensions.rst | ||
---|---|---|
456 | I'm not all too familiar with this area, but should "fat-binary" be hyphenated? I'd have thought it would have been two separate words (since the adjective "fat" isn't tied to the noun "binary"). OTOH, if that's how it's always referred to, I guess it's fine. | |
461 | ||
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp | ||
504–506 | Unfortunately, I don't really know CodeGen personally, so I can't advise - someone else needs to comment here. Unrelated to that discussion, but is this line tested? (It might be, but I'd somewhat expect to see a test case which checks the section type of an IR-produced section which doesn't go via asm or something like that - (this may all be nonsense due to lack of familiarity). |
Adding test and fixing grammar.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp | ||
---|---|---|
504–506 | it should be tested by the offload_sections test, showing that the @llvm_offloading type shows up on the global for ELF. We have some similar code above that does something similar for adding the SHF_EXCLUDE flag. |
I'm just trying to understand the test coverage, because I'm not too familiar with the touched areas of code:
- CodeGen/TargetLoweringObjectFileImpl.cpp is covered by CodeGen/X86/offload_sections.ll? (CodeGen converts named section type to ELF type)
- MCParser/ELFAsmParser.cpp is covered by MC/AsmParser/llvm_section_types.s (llvm-mc can understand the section directive)
- MCSectionELF.cpp is covered by CodeGen/X86/offload_sections.ll? (Code converts ELF type for section directive)
- Object/ELF.cpp is covered by MC/AsmParser/llvm_section_types.s (llvm-readobj recognises the section type)
- ObjectYAML/ELFYAML.cpp is covered by ObjectYAML/ELF/sht-offloading.yaml
Does this line up with your expectation? If so, LGTM. (If not, please could you explain where it doesn't!)
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp | ||
---|---|---|
504–506 | Thanks, pulling this discussion out-of-line. |
Addendum: it might be worth getting somebody else to chime in on the CodeGen change before landing this.
Yes, that sounds right. Thanks for the reviews
Yes, I would like to get some feedback on if I should go with some metadata attached to the generated global to determine if it should be at this section. I mainly want this so I can decide whether or not this section should have SHF_EXCLUDE or not.
I'm not all too familiar with this area, but should "fat-binary" be hyphenated? I'd have thought it would have been two separate words (since the adjective "fat" isn't tied to the noun "binary"). OTOH, if that's how it's always referred to, I guess it's fine.