llvm dev - Feb 2010 - [LLVMdev] 2nd attempt for a working patch for bug 2606

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>

[sidenote: Please try to avoid extraneous whitespace and line wrapping
changes in your patches. It makes it harder to see what you're actually
changing]

On Fri, Feb 26, 2010 at 4:57 AM, Garrison Venn <gvenn.cfe.dev at
gmail.com>wrote:
> 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). ...
>
Have you tried the existing pending functions mechanism? I don't want to
introduce stubs in eager compilation mode, and I don't think you have to.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100226/b04377dd/attachment.html>

Hi Jeffrey,
On Feb 26, 2010, at 16:02, Jeffrey Yasskin wrote:
> [sidenote: Please try to avoid extraneous whitespace and line wrapping
changes in your patches. It makes it harder to see what you're actually
changing]
Sorry just saw some preexisting code was not in 80
columns.
> 
> On Fri, Feb 26, 2010 at 4:57 AM, Garrison Venn <gvenn.cfe.dev at
gmail.com> wrote:
> 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). ...
> 
> Have you tried the existing pending functions mechanism? I don't want
to introduce stubs in eager compilation mode, and I don't think you have to.
I'm not sure I understand as forceEmitFunctionStub(...) merely calls
getLazyFunctionStub(...) which uses the pending function mechanism. The
stubs are replaced at compile (JIT) time not at runtime. Is this not how call
sites are currently treated? Is this what you are referring to?

Thanks for looking at this

Garrison

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