rsmith added inline comments.
================ Comment at: lib/CodeGen/CGCUDANV.cpp:281 + // get name from the module to generate unique ctor name for every module + SmallString<128> ModuleName ---------------- rjmccall wrote: > v.g.vassilev wrote: > > rjmccall wrote: > > > SimeonEhrig wrote: > > > > tra wrote: > > > > > SimeonEhrig wrote: > > > > > > rjmccall wrote: > > > > > > > Please explain in the comment *why* you're doing this. It's just > > > > > > > for debugging, right? So that it's known which object file the > > > > > > > constructor function comes from. > > > > > > The motivation is the same at this review: > > > > > > https://reviews.llvm.org/D34059 > > > > > > We try to enable incremental compiling of cuda runtime code, so we > > > > > > need unique ctor/dtor names, to handle the cuda device code over > > > > > > different modules. > > > > > I'm also interested in in the motivation for this change. > > > > > > > > > > Also, if the goal is to have an unique module identifier, would > > > > > compiling two different files with the same name be a problem? If the > > > > > goal is to help identifying a module, this may be OK, if not ideal. > > > > > If you really need to have unique name, then you may need to do > > > > > something more elaborate. NVCC appears to use some random number (or > > > > > hash of something?) for that. > > > > We need this modification for our C++-interpreter Cling, which we want > > > > to expand to interpret CUDA runtime code. Effective, it's a jit, which > > > > read in line by line the program code. Every line get his own > > > > llvm::Module. The Interpreter works with incremental and lazy > > > > compilation. Because the lazy compilation, we needs this modification. > > > > In the CUDA mode, clang generates for every module an _ > > > > _cuda_module_ctor and _ _cuda_module_dtor, if the compiler was started > > > > with a path to a fatbinary file. But the ctor is also depend on the > > > > source code, which will translate to llvm IR in the module. For > > > > Example, if a _ _global_ _ kernel will defined, the CodeGen add the > > > > function call __cuda_register_globals() to the ctor. But the lazy > > > > compilations prevents, that we can translate a function, which is > > > > already translate. Without the modification, the interpreter things, > > > > that the ctor is always same and use the first translation of the > > > > function, which was generate. Therefore, it is impossible to add new > > > > kernels. > > > I'm not asking you to explain to *me* why you're doing this, I'm asking > > > you to explain *in the comment* why you're doing this. > > > > > > That said, we should discuss this. It sounds like you need the function > > > to have a unique name because otherwise you're seeing inter-module > > > conflicts between incremental slices. Since the function is emitted with > > > internal linkage, I assume that those conflicts must be because you're > > > promoting internal linkage to external in order to make incremental > > > processing able to link to declarations from an earlier slice of the > > > translation unit. I really think that a better solution would be to > > > change how we assign LLVM linkage to static global declarations in IRGen > > > — basically, recognizing the difference between internal linkage (where > > > different parts of the translation unit can still refer to the same > > > entity) and no linkage at all (where they cannot). We could then > > > continue to emit truly private entities, like global ctors/dtors, lambda > > > bodies, block functions, and so on, with internal/private linkage without > > > worrying about how your pass will mess up the linkage later. > > @rjmccall, I agree. What's the best way to discuss this? My irc handle is > > vvassilev and I am in CET timezone. I will be online for approx. 2 hours > > from now on. > Sorry, I seem to have missed you for today. I think for the next day it > would be best to just trade e-mail, because I have errands to run in the > morning and early afternoon tomorrow. > > I think the major piece of the plan would be to make things like the > computation of GVALinkage in ASTContext.cpp consider your incremental mode. > Currently, `basicGVALinkageForFunction` and `basicGVALinkageForVariable` only > consider `isExternallyVisible()`, which conflates no-linkage and > internal-linkage; you would need to map internal linkage to > `GVA_StrongExternal` when processing in incremental mode. > > That alone might not be sufficient because there are things with no formal > linkage that still do need to be shared across incremental slices; for > example, anonymous structures at global scope. To get those things right, we > will need to split NoLinkage by adding an InternalNoLinkage for declarations > that formally have no linkage but in reality are visible throughout the > translation unit; but that seems quite feasible. > > We should get Richard's thoughts on that plan first, though. I'll be busy with the C++ committee meeting for at least the rest of the week, sorry for any latency here. The case where a type has no linkage and no stable mangling seems to also require persisting some kind of Decl -> IR module mangling (a mangle numbering context for the whole TU maybe?) in addition to linkage promotion. The linkage promotion itself seems reasonable to me, and doing it at the GVALinkage level seems consistent with how we handle some related cases (eg, Modules TS linkage promotion). Repository: rC Clang https://reviews.llvm.org/D44435 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
