[PATCH] D44435: CUDA ctor/dtor Module-Unique Symbol Name

Fri May 4 10:24:22 PDT 2018

tra added a comment.

Perhaps we should take a step back and consider whether this is the right approach to solve your problem.

If I understand it correctly, the real issue is that you repeatedly recompile the same module and cling will only use the function from the first module it's seen it in. Unlike regular functions that presumably remain the same in all the modules they are present in, CUDA constructors do change and you need cling to grab the one from the most recent module.

This patch deals with the issue by attempting to add a unique sufix. Presumably cling will then generate some sort of unique module name and will get unique constructor name in return. The down side of this approach is that module name is something that is derived from the file name and the functionality you're changing is in the shared code, so you need to make sure that whatever you implement makes sense for LLVM in general and that it does what it claims it does. AFAICT, LLVM has no pressing need for the unique constructor name -- it's a function with internal linkage and, if we ever need to generate more than one, LLVM is capable of generating unique names within the module all by itself. The patch currently does not fulfill the "unique" part either.

Perhaps you should consider a different approach which could handle the issue completely in cling. E.g. You could rename the constructor in the module's IR before passing it to JIT. Or you could rename it in PTX (it's just text after all) before passing it to driver or PTXAS.

================
Comment at: lib/CodeGen/CGCUDANV.cpp:287
+    CtorSuffix.append("_");
+    CtorSuffix.append(ModuleName);
+  }
----------------
SimeonEhrig wrote:
> tra wrote:
> > SimeonEhrig wrote:
> > > tra wrote:
> > > > There is a general problem with this approach. File name can contain the characters that PTX does not allow.
> > > > We currently only deal with '.' and '@', but that's not enough here.
> > > > You may want to either mangle the name somehow to avoid/convert illegal characters or use some other way to provide unique suffix. Hex-encoded hash of the file name would avoid this problem, for example.
> > > > 
> > > > 
> > > > 
> > > Maybe I'm wrong but I think, that should be no problem, because the generating of a cuda ctor/dtor have nothing to do with the PTX generation. 
> > > 
> > > The function 'makeModuleCtorFunction' should just generate llvm ir code for the host (e.g. x86_64).
> > > 
> > > If I'm wrong, could you tell me please, where in the source code the 'makeModuleCtorFunction' affect the PTX generation.
> > You are correct that PTX is irrelevant here. I've completely missed that this will be generated for the host, which is more forgiving. 
> > 
> > That said, I'm still not completely sure whether we're guaranteed that using arbitrary characters in a symbol name is OK on x86 and, potentially, other host platforms. As an experiment, try using a module which has a space in its name.
> At line 295 and 380 in CGCUDANV.cpp I use a sanitizer function, which replace all symbols without [a-zA-Z0-9._] with a '_'. It's the same solution like in D34059. So I think, it would works in general.
> 
> Only for information. I tested it with a module name, which includes a whitespace and without the sanitizer. It works on Linux x86 and the ELF format. There was an whitespace in the symbol of the cuda module ctor (I checked it with readelf).
> 
> In general, do you think my solution approach is technically okay? Your answer will be really helpful for internal usage in our cling project. At the moment I developed the cling-cuda-interpreter based on this patch and it would helps a lot of, if I can say, that the patch doesn't cause any problem with the CUDA-environment.  
This still does not give you unique suffix which was stated at the main goal of this patch. E.g files "foo$bar" and "foo_bar" will produce identical names. See my previous comment regarding using a hash.

https://reviews.llvm.org/D44435