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

[Joseph Huber via Phabricator via cfe-commits]

jhuber6 added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.h:279
 
+  /// List of file passed with -fembed-offload-binary option to embed
+  /// device-side offloading binaries in the host object file.
----------------
JonChesterfield wrote:
> 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>>?
It's a list of comma separated values, but you're right. This should be parsed out when we handle the flags.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1757
+
+  assert(BinaryFilenames.size() == BinarySections.size() &&
+         "Different number of filenames and section names in embedding");
----------------
JonChesterfield wrote:
> Definitely don't want to assert on invalid commandline argument
Will remove.


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


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4982
+  Type *UsedElementType = Type::getInt8Ty(M.getContext())->getPointerTo(0);
+  GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true);
+  for (auto *GV : UsedGlobals) {
----------------
JonChesterfield wrote:
> 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?
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.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4994
+
+  // Embed the data in the
+  llvm::Constant *ModuleConstant =
----------------
JonChesterfield wrote:
> missing end of comment
Will fix.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:5003
+  // sections after linking.
+  GV->setAlignment(Align(1));
+  UsedArray.push_back(
----------------
JonChesterfield wrote:
> Is this necessary? 1 seems a likely default for a uint8_t array
Will remove.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116542