[LLVMdev] SmallPtrSet patch for MCJIT

[Kaylor, Andrew]

Hi Yaron,

In order to preserve the behavior of the current implementation, both FindFunctionNamed and runStaticConstructorsDestructors would need to operate on the entire set of modules, not just the finalized modules.

In the case of FindFunctionNamed this is definitely the correct behavior since clients may use this function to find a function to be run without knowing which module it belongs to (and therefore without knowing whether or not code has been generated for the module).

The case of runStaticConstructorsDestructors is a bit more complicated.  Since multiple module support in MCJIT is such a recent addition there are probably no clients depending on the existing behavior (which would trigger the finalization of any module containing static containing static constructors and destructors.  On the other hand, we don't want to require the client to know which modules contain static constructors/destructors and invoke this function each time one is about to be used.  We can't automatically invoke this function anywhere because MCJIT does not know whether or not the target is local.  That is, the code might have been generated for a remote target.

For the above reasons, I think that runStaticConstructorsDestructors also needs to operate on all known modules and not just the finalized modules.

Feel free to commit this patch after making the above changes.

Thanks,
Andy


From: Yaron Keren [mailto:yaron.keren at gmail.com]
Sent: Tuesday, October 22, 2013 4:15 PM
To: Kaylor, Andrew
Cc: <llvmdev at cs.uiuc.edu>
Subject: Re: SmallPtrSet patch for MCJIT

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<mailto: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<mailto:yaron.keren at gmail.com>]
Sent: Monday, October 21, 2013 10:57 PM
To: <llvmdev at cs.uiuc.edu<mailto: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/ebada00b/attachment.html>