[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1bae02b77335: [Cuda] Use fallback method to mangle externalized decls if no CUID given (authored by jhuber6). Repository: rG LLVM Github Monorepo

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 updated this revision to Diff 432146. jhuber6 added a comment. Add test for #line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125904/new/ https://reviews.llvm.org/D125904 Files: clang/lib/CodeGen/CGCUDANV.cpp

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. In D125904#3538742 , @jhuber6 wrote: > In D125904#3538714 , @tra wrote: > >> It would be great to have some compile-time checks for that, if possible. >> Otherwise it will only manifest at

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3538714 , @tra wrote: > It would be great to have some compile-time checks for that, if possible. > Otherwise it will only manifest at run-time and the end user will have no > clue what's going on. Not sure how we

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. In D125904#3538163 , @yaxunl wrote: > I am OK with this patch. OK. > That said, this patch provided a default CUID that do not depend on driver, > which is its advantage. It should be OK for most usecases. I agree with this part.

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

yaxunl added a comment. In D125904#3537952 , @tra wrote: > How much work would it take to add cuid generation in the new driver, similar > to what the old driver does, using the same logic, however imperfect it is? > I'd be OK with that as a possibly

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3537952 , @tra wrote: > How much work would it take to add cuid generation in the new driver, similar > to what the old driver does, using the same logic, however imperfect it is? > I'd be OK with that as a possibly

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. How much work would it take to add cuid generation in the new driver, similar to what the old driver does, using the same logic, however imperfect it is? I'd be OK with that as a possibly permanent solution. I'm somewhat wary of temporary solutions as they tend to become

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3537872 , @tra wrote: > Is this patch in its current form blocking any of your other work? no-cuid > approach, even if we figure out how to do it, will likely take some time. Do > you need an interim solution until

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. Is this patch in its current form blocking any of your other work? no-cuid approach, even if we figure out how to do it, will likely take some time. Do you need an interim solution until then? In D125904#3537385 , @jhuber6 wrote:

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. Also, for the OpenMP case, we already pass the host-IR as a dependency for the device compilation. So it would be relatively easy for us to just generate these names on the host and then read them from the IR for the device. The problem is that CUDA / HIP doesn't use

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3535905 , @jhuber6 wrote: > I can't think of a way to generate these new symbols, we'd need to somehow > have a list of all the static entries that need new symbols and then modify > the object file after its been

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3535830 , @tra wrote: > I'm still itching to figure out a way to avoid CUID altogether and with the > new driver it may be possible. I would be 100% in favor of working around this if possible, it's proving to be

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. I'm still itching to figure out a way to avoid CUID altogether and with the new driver it may be possible. CUID serves two purposes: a) avoid name conflicts during device-side linking ("must be globally unique" part) b) allow host to refer to something in the GPU executable

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. A Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6836 + + // If the CUID is not specified we try to generate a unique postfix. + if (getLangOpts().CUID.empty()) { jhuber6 wrote: > tra wrote: > > jhuber6 wrote: > > > tra wrote: > > >

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6836 + + // If the CUID is not specified we try to generate a unique postfix. + if (getLangOpts().CUID.empty()) { tra wrote: > jhuber6 wrote: > > tra wrote: > > > > However, [CUID]

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6836 + + // If the CUID is not specified we try to generate a unique postfix. + if (getLangOpts().CUID.empty()) { jhuber6 wrote: > tra wrote: > > > However, [CUID] is not always

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6836 + + // If the CUID is not specified we try to generate a unique postfix. + if (getLangOpts().CUID.empty()) { tra wrote: > > However, [CUID] is not always availible. > > The

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6836 + + // If the CUID is not specified we try to generate a unique postfix. + if (getLangOpts().CUID.empty()) { > However, [CUID] is not always availible. The question is -- when

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 updated this revision to Diff 431702. jhuber6 added a comment. Adding extra commentto mention the hidden requirement that the driver shuold not define a different `-D` option for the host and device. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6845-6846 +llvm::MD5::MD5Result Result; +for (const auto : PreprocessorOpts.Macros) + Hash.update(Arg.first); +Hash.final(Result); yaxunl wrote: > jhuber6 wrote: > >

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

yaxunl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6845-6846 +llvm::MD5::MD5Result Result; +for (const auto : PreprocessorOpts.Macros) + Hash.update(Arg.first); +Hash.final(Result); jhuber6 wrote: > yaxunl wrote: > >

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6845-6846 +llvm::MD5::MD5Result Result; +for (const auto : PreprocessorOpts.Macros) + Hash.update(Arg.first); +Hash.final(Result); yaxunl wrote: > Are these options

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

yaxunl added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6845-6846 +llvm::MD5::MD5Result Result; +for (const auto : PreprocessorOpts.Macros) + Hash.update(Arg.first); +Hash.final(Result); Are these options always the same

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 updated this revision to Diff 431666. jhuber6 added a comment. Removing use of the line number, instead replacing it with an 8 character wide hash of the `-D` options passed to the front-end. This should make it sufficiently unique for users compiling the same file with different

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3532608 , @tra wrote: > That said, I would consider compiling the same source with different > preprocessor options to be a legitimate use case that we should support. > Explicitly passing cuid would work as a

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. In D125904#3532545 , @jhuber6 wrote: > clang a.c -c -o a-0.o // Has some externalized static variable. > clang a.c -c -o a-1.o > clang a-0.o a-1.o // Redefined symbol error Ah. OK. This is a bit less of a concern. As long as we take

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. In D125904#3532492 , @tra wrote: > This is a moderately serious issue. Some users care about the build > reproducibility. Recompiling the same sources and getting different results > will trigger all sorts of red flags that

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

tra added a comment. > The one downside to this is that we are no longer stable under multiple > compilations of the same file. This is a moderately serious issue. Some users care about the build reproducibility. Recompiling the same sources and getting different results will trigger all

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 added a comment. Ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125904/new/ https://reviews.llvm.org/D125904 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D125904: [Cuda] Use fallback method to mangle externalized decls if no CUID given

jhuber6 created this revision. jhuber6 added reviewers: yaxunl, tra. Herald added subscribers: mattd, carlosgalvezp. Herald added a project: All. jhuber6 requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. CUDA requires that static variables