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

[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>

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