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

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

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.



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1774
+      SectionName += ".";
+      SectionName += *BinarySection;
+    }
----------------
jhuber6 wrote:
> JonChesterfield wrote:
> > 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?
> 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.
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.


================
Comment at: clang/test/Frontend/embed-object.ll:2
+; RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm \
+; RUN:    -fembed-offload-object=%S/Inputs/empty.h,section -x ir %s -o - \
+; RUN:    | FileCheck %s -check-prefix=CHECK
----------------
I think we need a test case with more than one embedded file, given there's the careful splitting around commas in the implementation


================
Comment at: llvm/lib/Bitcode/Writer/CMakeLists.txt:14
   MC
+  TransformUtils
   Object
----------------
jhuber6 wrote:
> I'm not sure if it's worth linking TransformUtils just for `appendToCompilerUsed` I can copy the implementation locally if not.
Definitely better to link in an LLVM library than to copy&paste functions out of it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116542