[PATCH] D129052: [Object] Add ELF section type for offloading objects

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.
This revision is now accepted and ready to land.

I'm just trying to understand the test coverage, because I'm not too familiar with the touched areas of code:

1. `CodeGen/TargetLoweringObjectFileImpl.cpp` is covered by `CodeGen/X86/offload_sections.ll`? (CodeGen converts named section type to ELF type)
2. `MCParser/ELFAsmParser.cpp` is covered by `MC/AsmParser/llvm_section_types.s` (llvm-mc can understand the section directive)
3. `MCSectionELF.cpp` is covered by `CodeGen/X86/offload_sections.ll`? (Code converts ELF type for section directive)
4. `Object/ELF.cpp` is covered by `MC/AsmParser/llvm_section_types.s` (llvm-readobj recognises the section type)
5. `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!)



================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:504-506
+  if (hasPrefix(Name, ".llvm.offloading"))
+    return ELF::SHT_LLVM_OFFLOADING;
+
----------------
jhuber6 wrote:
> jhenderson wrote:
> > jhuber6 wrote:
> > > 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.
> > 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).
> 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.
Thanks, pulling this discussion out-of-line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129052/new/

https://reviews.llvm.org/D129052