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

On Fri, Feb 26, 2010 at 1:26 PM, Garrison Venn <gvenn.cfe.dev at
gmail.com> wrote:
> 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?
Ah, I was confused. You're right, getLazyFunctionStub is the
badly-named function you want.

You currently have nearly the same loop in getOrEmitGlobalVariable()
and getPointerToFunction(). Can you unify those, perhaps by making
them searches for GlobalValues with getNamedValue()?

GetLinkageResult() in lib/Linker/LinkModules.cpp has a lot of logic
about choosing a function based on its linkage. You should read that
to figure out how to filter GVs with various linkages in your search.

Please add some tests for cross-module JITting. You'll find some of
the functions defined in unittests/ExecutionEngine/JIT/JITTest.cpp
helpful, but I think this belongs in a separate file. Just copy the
useful functions, and at some point I'll build an actual library for
the helper functions.



On Fri, Feb 19, 2010 at 8:21 AM, Garrison Venn <gvenn.cfe.dev at
gmail.com> wrote:
> Major mods:
> JIT.{h,cpp}:
> 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 suspect you can pull the decision of how to emit a stub back into
the JITEmitter. If you add a
"Function*findDefiningFunction(Function*)" method to the JIT, which
searches the Modules for the strongest definition of a same-named
function, you can keep the logic for how to deal with compiling that
function in the JITEmitter.
> I also kept the hasAvailableExternallyLinkage() the same as
> before, as I'm not sure if this
> should also go through the same module procedure.
If something's available_externally in the current module and external
in another module, I think we should compile the external definition.
If it's available_externally in all modules, we need to look with
dlsym(). Please add a test for that.
> 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?
Yes, I think you should do the same thing LinkModules.cpp does when
it's possible.

> 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.
Could you add a test in which M1::F1 uses M2::GVar1, which contains in
its initializer the address of M1::F2?
> 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.
That doesn't seem right.
> 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?
They may not have exactly the same type because of opaque types in
each module. We probably shouldn't go to the trouble of unifying the
types between the modules, so I'd be inclined to just ignore the
types. Or maybe only pay attention when they're concrete types?
> 4)    The declaration of void* forceEmitFunctionStub(Function *F) was added
> to the class JIT. This is
> implemented in JITEmitter.cpp.
This isn't quite the right name, but I think you can eliminate it
entirely (see above).
> Beyond any issues with the patch, there is a question, in my mind, as to
> whether 2606 is really a bug.
I suspect it is.
> 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.
If I understand correctly, the current manual solution is to figure
out what functions the JIT is going to want from another module, and
compile them first. That would continue to work even after the JIT
links modules for you.
> 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".
This strikes me as overkill until someone actually needs it.


Thanks,
Jeffrey

Hi Jeffrey,

I'm still in the process of learning the unittest technology. While I did
get distracted with
test-suite and test, and therefore the requisite installs of llvm-gcc, and
DejaGNU, I think
I'm back on track now. So that was a waste of time. By the way the reference
I have to the
unittests target was in an archive for this list. Is there no llvm level doc for
this? Regardless
the gtest.h is fairly easy to understand. It just seems like one can get
distracted by the
lack of llvm doc. on unittests when a corresponding doc exists for the testing
infrastructure.
The developers guide did not seem to contain anything either. If there is such a
doc, could
you give me a link?


On Feb 26, 2010, at 21:17, Jeffrey Yasskin wrote:
> 
> You currently have nearly the same loop in getOrEmitGlobalVariable()
> and getPointerToFunction(). Can you unify those, perhaps by making
> them searches for GlobalValues with getNamedValue()?
> 
> GetLinkageResult() in lib/Linker/LinkModules.cpp has a lot of logic
> about choosing a function based on its linkage. You should read that
> to figure out how to filter GVs with various linkages in your search.
When I'm able to write a unit test, I'll re-submit a patch which will
contain
a JIT function to handle GlobalValues (both GlobalVariables and Functions).
I did this for that CrossModuleJIT experiment that I posted in another thread.
Anyway I'll start from there, and then proceed to using LinkModules.cpp
logic.

[snip]
> 
> Please add some tests for cross-module JITting. You'll find some of
> the functions defined in unittests/ExecutionEngine/JIT/JITTest.cpp
> helpful, but I think this belongs in a separate file. Just copy the
> useful functions, and at some point I'll build an actual library for
> the helper functions.
Working on this now.

[snip]
> 
> I suspect you can pull the decision of how to emit a stub back into
> the JITEmitter. If you add a
> "Function*findDefiningFunction(Function*)" method to the JIT,
which
> searches the Modules for the strongest definition of a same-named
> function, you can keep the logic for how to deal with compiling that
> function in the JITEmitter.
Will do
> 
>> I also kept the hasAvailableExternallyLinkage() the same as
>> before, as I'm not sure if this
>> should also go through the same module procedure.
> 
> If something's available_externally in the current module and external
> in another module, I think we should compile the external definition.
> If it's available_externally in all modules, we need to look with
> dlsym(). Please add a test for that.
Will do
> 
>> 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?
> 
> Yes, I think you should do the same thing LinkModules.cpp does when
> it's possible.
Yes I'll create one function to treat all GlobalValues and then reuse
LinkModules
logic. I've yet to look at LinkModules.cpp

[snip]
> 
> Could you add a test in which M1::F1 uses M2::GVar1, which contains in
> its initializer the address of M1::F2?
> 
Will do
>> 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.
> 
> That doesn't seem right.
Ok I'll check on my understanding. On thinking about it, I think I see how
I'm
mistaken.
> 
>> 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?
> 
> They may not have exactly the same type because of opaque types in
> each module. We probably shouldn't go to the trouble of unifying the
> types between the modules, so I'd be inclined to just ignore the
> types. Or maybe only pay attention when they're concrete types?
Ok, I'll modify accordingly if I understand how.
> 
>> 4)    The declaration of void* forceEmitFunctionStub(Function *F) was
added
>> to the class JIT. This is
>> implemented in JITEmitter.cpp.
> 
> This isn't quite the right name, but I think you can eliminate it
> entirely (see above).
Yup
> 
>> Beyond any issues with the patch, there is a question, in my mind, as
to
>> whether 2606 is really a bug.
> 
> I suspect it is.
Cool, then we are moving forward.
> 
>> 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.
> 
> If I understand correctly, the current manual solution is to figure
> out what functions the JIT is going to want from another module, and
> compile them first. That would continue to work even after the JIT
> links modules for you.
Yes, in re-thinking this, I believe you are correct. I'm not sure about
all the use cases though, but you have much more knowledge on this
than I do. So I'll ignore and proceed.
> 
>> 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".
> 
> This strikes me as overkill until someone actually needs it.
Yes, most definitely
> 
> 
> Thanks,
> Jeffrey
Thanks again. By the way if I'm too slow on this, don't hesitate to jump
in. I have to
learn this process anyway, and this bug is a good way to do it. So I'll
proceed in my
own private space even if you, Oliver, or someone else shotguns ahead. 

I do have one question though. After having done the fix and the appropriate
unit tests,
does one still need to add to the tests suite for the testing bots, and add to
test for make
check (for this kind of bug fix)? Do the unittests somehow get invoked by make
check? I
guess I don't really understand the relation between the tests exercised by
the unittests
target, and the tests exercised by the check target. Maybe there isn't any.

Garrison

On Mon, Mar 1, 2010 at 4:52 AM, Garrison Venn <gvenn.cfe.dev at gmail.com>
wrote:
> I do have one question though. After having done the fix and the
appropriate unit tests,
> does one still need to add to the tests suite for the testing bots, and add
to test for make
> check (for this kind of bug fix)? Do the unittests somehow get invoked by
make check? I
> guess I don't really understand the relation between the tests
exercised by the unittests
> target, and the tests exercised by the check target. Maybe there isn't
any.
The unit tests do get run by `make check-lit` and by the build bots. I
suspect they're not run by `make check`, but check-lit is faster
anyway. A unit test is sufficient for this change (and maybe all you
can possibly write given that it requires multiple modules in the same
JIT, which lli isn't set up to do.)

Sorry they aren't better documented.