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