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

MaskRay added a comment. In D116542#3285985 , @jhuber6 wrote: > In D116542#3285983 , @MaskRay wrote: > >> @jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9 >>

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

jhuber6 added a comment. In D116542#3285983 , @MaskRay wrote: > @jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9 > .. OK, I > was late. > > See D118666

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

MaskRay added a comment. @jhuber6 Please don't do 4a780aa13ee5e1c8268de54ef946200a270127b9 .. OK, I was late. See D118666 for the proper fix. I'd be better to revert this relevant changes if

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

jhuber6 added a comment. In D116542#3285857 , @cmtice wrote: > This change introduces a circular dependency: BitcodeWriters now depends on > TransformUtils, but TransformUtils also depends on BitcodeWriters. This > appears to be a layering violation.

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

cmtice added a comment. This change introduces a circular dependency: BitcodeWriters now depends on TransformUtils, but TransformUtils also depends on BitcodeWriters. This appears to be a layering violation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG551b17745244: [OpenMP] Add a flag for embedding a file into the module (authored by jhuber6). Changed prior to commit: https://reviews.llvm.org/D1

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

JonChesterfield accepted this revision. JonChesterfield added a comment. This revision is now accepted and ready to land. Thanks for sticking with me on this! LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D11

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

jhuber6 updated this revision to Diff 404506. jhuber6 added a comment. Add error handling routine to ensure that the embedding string is always a pair separated by a single ','. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://rev

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1760 + for (StringRef OffloadObject : CGOpts.OffloadObjects) { +auto FilenameAndSection = OffloadObject.split(','); +llvm::ErrorOr> ObjectOrErr = JonChesterfield wrote:

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1771 +SmallString<128> SectionName( +{".llvm.offloading.", std::get<1>(FilenameAndSection)}); +llvm::EmbedBufferInModule(*M, **ObjectOrErr, SectionName); OK, so o

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

JonChesterfield added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1760 + for (StringRef OffloadObject : CGOpts.OffloadObjects) { +auto FilenameAndSection = OffloadObject.split(','); +llvm::ErrorOr> ObjectOrErr = Could we have a type he

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

jhuber6 updated this revision to Diff 404047. jhuber6 added a comment. Changing the name to be the section name. This ensures that if the sections get merged we will get a linker error without failing silently. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

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

jhuber6 updated this revision to Diff 404037. jhuber6 added a comment. Adding test for multiple files (added it to wrong commit). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D116542 Files: clang/include/cla

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

jhuber6 added inline comments. 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

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

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1774 + SectionName += "."; + SectionName += *BinarySection; +} JonChesterfield wrote: > jhuber6 wrote: > > JonChesterfield wrote: > > > This looks lossy - if two files use

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

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 t

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

jhuber6 updated this revision to Diff 403476. jhuber6 added a comment. Forgot to rename file. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D116542 Files: clang/include/clang/Basic/CodeGenOptions.h clang/in

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

jhuber6 updated this revision to Diff 403468. jhuber6 added a comment. clang format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D116542 Files: clang/include/clang/Basic/CodeGenOptions.h clang/include/cla

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

jhuber6 added inline comments. Comment at: llvm/lib/Bitcode/Writer/CMakeLists.txt:14 MC + TransformUtils Object I'm not sure if it's worth linking TransformUtils just for `appendToCompilerUsed` I can copy the implementation locally if not. Repository:

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

jhuber6 updated this revision to Diff 403404. jhuber6 added a comment. Herald added a subscriber: mgorny. Updating approach, use a vector of string pairs now. Multiple files are simply passed multiple times. Will add filename to the offloading section name laterf, as similar sections could be merg

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

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 unc

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

JonChesterfield added inline comments. Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4934 assert(Old->hasOneUse() && "llvm.embedded.module can only be used once in llvm.compiler.used"); GV->takeName(Old); here's an assert that this f

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

JonChesterfield added inline comments. Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4891 GlobalVariable *Used = collectUsedGlobalVariables(M, UsedGlobals, true); for (auto *GV : UsedGlobals) { if (GV->getName() != "llvm.embedded.module" && Thi

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

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/C

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

jhuber6 updated this revision to Diff 397590. jhuber6 added a comment. Clang format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116542/new/ https://reviews.llvm.org/D116542 Files: clang/include/clang/Basic/CodeGenOptions.h clang/include/clan

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

jhuber6 created this revision. jhuber6 added reviewers: jdoerfert, gregrodgers, JonChesterfield, ronlieb. Herald added subscribers: ormris, dexonsmith, dang, guansong, hiraditya, yaxunl. jhuber6 requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, sstefan1. Herald