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

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



	

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100219/316ad368/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR2606.proposal.patch
Type: application/octet-stream
Size: 6962 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100219/316ad368/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20100219/316ad368/attachment-0001.html>

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 ?

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.

What do you think ? Anyone have an opinion on this ?

Olivier.

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 <http://llvm.org/bugs/show_bug.cgi?id=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 <http://llvm.org/bugs/show_bug.cgi?id=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<http://llvm.org/bugs/show_bug.cgi?id=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 <http://llvm.org/bugs/show_bug.cgi?id=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/20100225/144acec0/attachment.html>

On Thu, Feb 25, 2010 at 11:10 AM, Olivier Meurant
<meurant.olivier at gmail.com> 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 ?
>
> 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.
The LazyFunctionCreator has to guarantee that it returns non-null for
existing functions or the JIT will abort. If you have recursion across
modules, and the LazyFunctionCreator tries to compile them, that'll
either fail or try to compile recursively, which also fails an
assertion. So I think the single JIT dealing with multiple modules
will work better.
> What do you think ? Anyone have an opinion on this ?
>
> Olivier.
>
> 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
>>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>

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>