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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
