[LLVMdev] SmallPtrSet patch for MCJIT

[Kaylor, Andrew]

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/20131022/06d83fbd/attachment.html>