[LLVMdev] 2nd attempt for a working patch for bug 2606

[Garrison Venn]

Hi Olivier,

On Feb 25, 2010, at 14:10, Olivier Meurant wrote:

> Hi Garrison,
> 
> I finally come back from holidays and take time to watch your patch.
> 
> I must say that I largely prefer this version over the previous one ! I like the reuse of getLazyFunctionStub, but I don't know if the forceEmitFunctionStub is still needed ?

JIT::forceEmitFunctionStub(...) was created to bring its implementation into JITEmitter file scope as JITResolver and 
therefore JITResolver::getLazyFunctionStub(...) are members of an anonymous namespace in that scope. However
I can instead use a pre-existing method: getPointerToFunctionOrStub(...) which is declared in ExecutionEngine
and overwritten in JIT (within JITEmitter file scope). Your response prompted my search. I've unit tested this and it works great. 
There is one caveat though. The implementation of JIT::getPointerToFunctionOrStub(...) first invokes ExecutionEngine::getPointerToGlobalIfAvailable(...)
behavior to check whether or not the Function instance is already actualized.  Subsequently JIT::getPointerToFunctionOrStub(...) 
calls JITResolver::getLazyFunctionStub(...). Given that ExecutionEngine::getPointerToGlobalIfAvailable(...) iterates through all known 
actualized functions, we would take a compile time hit here by using getPointerToFunctionOrStub(...) versus my forceEmitFunctionStub(...) 
since we know at that point in the call stack that we are already dealing with a declaration. I don't know if this is a viable concern, 
but if not I'd be happy to change the 2606 working patch to reflect use of getPointerToFunctionOrStub(...) in place of forceEmitFunctionStub(...). 
On the pro side, using getPointerToFunctionOrStub(...) does get rid of introducing a new method (forceEmitFunctionStub(...)), which will not 
have the same future design support as  getPointerToFunctionOrStub(...). We could also modify ExecutionEngine::getPointerToGlobalIfAvailable(...) to 
avoid the search if the function is a declaration. I personally don't care for this approach.

> 
> I thought about JIT and modules, and I wonder if we don't need to take it another way.
> Now we can create multiples JIT. What if we decide to simplify the JIT in allowing only one module per instance of JIT ? It will simplify greatly the code of the JIT.
> 
> If we need to jit 2 differents modules, we can create 2 JITs. If one module needs another module, we can create a LazyFunctionCreator (called by JIT::getPointerToNamedFunction) and easily resolved cross-module references.

I can't comment on this as I have not dealt with multiple JITs as you and Jeffrey have (for those reading this thread after the fact 
see Jeffrey's follow up response).

Like you though I am favoring a solution where the factory nature of EngineBuilder is leveraged. In my view we should be able
to create a desired ExecutionEngine via this pattern. In reality though, there are two existing factory semantics used for the 
construction of ExecutionEngines. One is the one given by the API in EngineBuilder, where the type of ExecutionEngine is
determined by the a priori EngineBuilder::Kind. The other is contained by the manual header file include mechanism; including 
llvm/include/ExecutionEngine/JIT.h and or llvm/include/ExecutionEngine/ExecutionEngine.h. Currently both of these methods
are used in conjunction to determine the final outcome. If however one wanted to include a derived class of JIT to optionally bring 
into being my behavioral fix for 2606, one would have to deal with these methods. I personally would create a new inlined virtual 
declaration in JIT, and use it in JIT::getPointerToFunction(...), in place of my 2606 mod, to effect the new cross module functionality
in this new JIT derived class instance. A simple hack resolution of  "resolve these methods" implies leveraging another include file 
to set ExecutionEngine::JITCtor while simultaneously handling JIT.cpp's static setting of this same variable. A better solution would 
be to redesign EngineBuilder to use say dlopen/dlload functionality to dynamically pull in the proper JIT derived class library. 
Anyway this is my 2 cents.

> 
> What do you think ? Anyone have an opinion on this ?
> 
> Olivier.

Garrison

> 
> On Fri, Feb 19, 2010 at 5:21 PM, Garrison Venn <gvenn.cfe.dev at gmail.com> wrote:
> This is the second version of a patch, which I recently attached to bug 2606, whose original version was
> modified to reflect the lists comments. Also please note the comment at the end of this email, which basically
> questions whether this bug is really a bug.
> 
> 1) To solve the foreign Module GlobalVariable problem, I modified JIT::getOrEmitGlobalVariable(...) to 
> 	directly attempt to map a found "external" GlobalVariable. 
> 2) To solve the foreign Module Function problem, 
> 	I modified JIT.{h,cpp} to emit a stub for a found "external" Function instance, when that foreign Module 
> 	defined Function has not yet been compiled.
> 
> All areas of interest are marked with // GMV Mod:
> 
> Again I invite those members of the list that have time, to review this proposal for comments/changes.
> 
> Major mods:
> 
> JIT.{h,cpp}:
> 
> 1)	I included Module.h.
> 
> 2)	JIT::getPointerToFunction(...) was modified to search for a non-mapped function in all "other" modules.
> 	If found a stub is emitted in the same manner as other in module functions found. This stub gets re-emitted, 
> 	toward the end of the JIT cycle.
> 
> 	One issue here is that functions that are external to all modules, have their mappings delayed by the
> 	above loop. I also kept the hasAvailableExternallyLinkage() the same as before, as I'm not sure if this
> 	should also go through the same module procedure. The stub emission will only take place for function
> 	definitions that are marked with external linkage. Should other cases be allowed? Should visibility also 
> 	be considered?
> 
> 3)	JIT::getOrEmitGlobalVariable(...) was modified to search for non-mapped global variables in all "other"
> 	modules. If found, the address of the foreign global variable is requested, and mapped. There is no
> 	delay here.
> 
> 	An attempt is first made to find the global outside of all modules, before attempting to find the function
> 	in the known modules. This is the reverse of logic used in #2. On searching, unlike for functions, a linkage 
> 	check was not made. Also the global variable declaration and definition must have the exact same type. 
> 	Is this the correct approach?
> 
> 4)    The declaration of void* forceEmitFunctionStub(Function *F) was added to the class JIT. This is 
> 	implemented in JITEmitter.cpp.
> 
> JITEmitter.cpp:
> 
> 1)	JIT::forceEmitFunctionStub(...) was added. It turns around and calls JITResolver::getLazyFunctionStub(...) which
> 	emits the foreign function as a stub.
> 
> Beyond any issues with the patch, there is a question, in my mind, as to whether 2606 is really a bug. Sure its resolution makes 
> using the JIT simpler for cross module behavior, but current manual solutions may be more fined grained in their approach in
> deciding how to treat such functions. If true a fix to 2606 would circumvent such handling. Should we instead add a new 
> semantic to EngineBuilder which would configure the JIT, or whatever else, to handle such cross module linkage, if such behavior
> is warranted? By linkage I mean mapping variables, and functions found in "external" modules, into the execution engine. For example, 
> one could devise a new function pass style metaphor which would be setup in the EngineBuilder instance. If it exists, the resulting
> JIT instance would use this new pass style to determine what to do with "foreign functions, and variables". However the first question 
> is, whether or not 2606 is a bug in the first place?
> 
> Thanks again for any time spent on this
> 
> Garrison
> 
> 
> 
> 
> 	
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100226/fea72ca2/attachment.html>