[PATCH] tidy the MCJIT tests now applyPermissions will invalidate cache

[Kaylor, Andrew]

You may be right about the naming of applyPermissions, and you are definitely right about the poor documentation.

I agree with Filip that when the MCJIT engine is being used the client should call finalizeObject rather than using applyPermissions directly.

My thinking with the interfaces is that both the module within the MCJIT engine and the memory within the memory manager go through a series of state transitions (which obviously need to be better documented).  In the execution engine it goes something like this:

raw module -> code and data generated (or loaded from cache) -> object image loaded into memory -> object prepared for execution

The third state above is necessarily vague and open as we need to allow for clients moving the memory around, and possibly copying it to an external process or a remote system.  We currently lack a well-defined method to transition from the first state to the second or the second to the third.  Right now, the second and third states are coupled inside MCJIT such that we always go straight into the third state as soon as we enter the second.  Several methods will implicitly trigger this transition (for instance, getPointerToFunction).  That's not by design so much as a historical legacy.  Right now, relocations are applied in both the third state and the fourth state.  Again, that's kind of sloppy and is just an artifact of how we got things working.  Ideally, I don't think relocations should be applied until we go into the fourth state.  The finalizeObject method is intended as the canonical way to get us into the fourth and final state.

Things are a bit simpler with the memory manager side of things.  The intention is that the memory manager will allocate memory as RW such that we can use it to load the object and get it ready for execution, then when applyPermissions is called the memory manager will do whatever is necessary to get things in a state that's ready for execution.  That's why the cache invalidation went there.  The name does obscure some of what's happening.  Something like 'finalizeMemory' or 'prepareForExecution' might be better.

I didn't intend for clients to call applyPermissions directly.  Clients that do so are likely to miss the relocation step in the execution engine when that gets moved out of the code generation process.

-Andy

From: David Tweed [mailto:david.tweed at arm.com]
Sent: Monday, May 13, 2013 9:01 AM
To: 'Filip Pizlo'
Cc: llvm-commits at cs.uiuc.edu; Kaylor, Andrew
Subject: RE: [PATCH] tidy the MCJIT tests now applyPermissions will invalidate cache

Hi,

Thanks for the review.

| Wouldn't it be better if the tests did the "right thing" and called TheJIT->finalizeObject()?

I think you're probably right. It would also be good if the finalizeObject() declaration had a comment mentioning that's what it's intended use is. AFAICS the only comment is the oblique reference in SectionMemoryManager.h.

|I'm not sure how much I like SectionMemoryManager::applyPermissions() being specified to also mean flushing cache, but I don't have a strong opinion on that.

I think that it may be that applyPermissions might not be quite the right name, but my basic reasoning is that it does do the cache invalidation inside the body, so there should be commentary documenting this fact (particularly since it's allowed for alternative implementations of the memory manager to be used). As you can probably tell, in addition to the professional interest in having MCJIT work well on ARM, I'm interested in having LLVM's JITing facilities be documented enough to be usable by "casual users" without them needing to become JIT implementation experts.

I'll wait a bit for other comments, then respin a patch.

Cheers,
Dave

-Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130513/d34ef262/attachment.html>