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

[Jon Chesterfield via Phabricator via llvm-commits]

JonChesterfield requested changes to this revision.
JonChesterfield added a comment.
This revision now requires changes to proceed.

I don't think this works for multiple files. The test only tries one and misses all the parsing handling.



================
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.
----------------
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>>?


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


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


================
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) {
----------------
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?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4983
+  GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true);
+  for (auto *GV : UsedGlobals) {
+    if (!GV->getName().startswith("llvm.embedded.object"))
----------------
This removes all variables with that prefix


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


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


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:5007
+
+  // Recreate llvm.compiler.used.
+  ArrayType *ATy = ArrayType::get(UsedElementType, UsedArray.size());
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116542