[LLVMdev] SmallPtrSet patch for MCJIT

[Yaron Keren]

Hi Andy,

I added the runStaticConstructorsDestructors and FindFunctionNamed
functions. This also required making them virtual in EE.h.

I'm not sure about FindFunctionNamed:
In addition to searching finalized modules, should it search Added and
Loaded modules?
If it finds a Function in these, should it compile and finalize it before
returning the Function* ?

I modified the implementation assert to test for ownsModule.
However, the hasModuleBeenAdded functionality is required to avoid
duplicate search in getPointerToFunction.  As you note the name is wrong, I
renamed the function to hasModuleBeenAddedButNotLoaded and cleaned the
logic at getPointerToFunction() to minimize searches for all cases.

pop_back() vs. clear() - since I knew there is one module only (we override
the AddModule so EE can't get another) that what "popped" into my mind.
clear() is just as good, changed.

EE, JIT, MCJIT inheritance - the interpreter and the JIT would also benefit
from a SmallPtrSet module manager rather than the current vector so if it's
possible to create a module manager that makes sense for everyone, it could
be moved back into EE along with the several duplicated functions.

I clarified this in ~MCJIT comments and ExecutionEngine.h comment.

As English is my second language, you're welcome to correct any errors and
clarify.

Patch attached.

Yaron



2013/10/22 Kaylor, Andrew <andrew.kaylor at intel.com>

>  Hi Yaron,****
>
> ** **
>
> Overall this looks great.****
>
> ** **
>
> There are a couple of remaining holes.  Specifically, MCJIT inherits
> implementations of the FindFunctionNamed and
> runStaticConstructorsDestructors methods from ExecutionEngine which use the
> old Modules vector, so you’ll need to override these in MCJIT.  (The
> implementations are fairly trivial.)****
>
> ** **
>
> Beyond that I just have a couple of questions.****
>
> ** **
>
> First, OwningModuleContainer::hasModuleBeenAdded seems wrong.  It seems to
> me that it should be equivalent to OwningModuleContain::ownsModule.  That
> is, everything that has been loaded or finalized has also been added.  Your
> implementation checks “has been added but not loaded” but it doesn’t seem
> like that’s necessary anywhere.  Can you eliminate this function and just
> use OwningModuleContain::ownsModule?****
>
> ** **
>
> Second, why did you decide to use “Modules.pop_back()” in the MCJIT
> destructor rather than “Modules.clear()”?  I expect there will only be one
> module that makes it into that list so they should be functionally
> equivalent, but calling “clear” seems to better indicate the intention.
> Also, in the same place, I think “don’t inherit from ExecutionEngine” is a
> potentially misleading comment, since that’s the class that clients
> actually use to interface with MCJIT.  It would probably be better to say
> something about disentangling the JIT and MCJIT interfaces and specifically
> mention that we don’t want the base class to manage our modules.  Keep in
> mind, however, that in addition to JIT and MCJIT, there’s also the
> Interpreter class which inherits from ExecutionEngine.****
>
> ** **
>
> There are a few very basic multiple module tests (multi-module-a.ll,
> multi-module-sm-pic-a.ll , multi-module-eh-a.ll and cross-module-a.ll in
> test/ExecutionEngine/MCJIT, as well as remote versions) that get run as
> part of the basic acceptance test suite.  These don’t test the case of
> running static constructors/destructors in secondary modules, but it
> shouldn’t be hard to add that.****
>
> ** **
>
> Definitely feel free to add more test cases!****
>
> ** **
>
> -Andy****
>
> ** **
>
> ** **
>
> *From:* Yaron Keren [mailto:yaron.keren at gmail.com]
> *Sent:* Monday, October 21, 2013 10:57 PM
> *To:* <llvmdev at cs.uiuc.edu>; Kaylor, Andrew
> *Subject:* SmallPtrSet patch for MCJIT****
>
> ** **
>
> Hi Andy,****
>
> ** **
>
> Here is the patch. it incorporates:****
>
> ** **
>
> 1) your latest patch to SVN.****
>
> 2) mcjit-module-state-optimization.patch.****
>
> 3) the PtrSet changes.****
>
> ** **
>
> Other than the OwnedModules implementation there were other differences
> between 1) and 2), especially in the Finalize* functions, so please review
> that I got the right code.****
>
> ** **
>
> I got bitten by subtle bugs arising from MCJIT inheriting from EE: ****
>
> First, MCJIT overridden addModule but not removeModule so the EE version
> was called. Second, both MCJIT and EE constructors take ownership of the
> module and their destructors try to delete it. Fixed this with a hack in
> MCJIT destructor and a FIXME comment, the real fix is simply getting EE out
> of the picture. It just interferes.****
>
> I know it's in the plans, yet more reasons to do so.****
>
> ** **
>
> Finally, I don't know if there are good multi-module test cases. I have
> run only my one-module use-case (currently - I'm having other-than-MCJIT
> issues with multiple modules) with the code and after fixing the above bugs
> the code, debugged line by line, appear to work OK.****
>
> ** **
>
> Yaron****
>
> ** **
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131023/379fc3df/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mcjit-ptrset2.diff
Type: application/octet-stream
Size: 17405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131023/379fc3df/attachment.obj>