[llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Danil Malyshev dmalyshev at accesssoftek.com
Wed Mar 28 13:08:53 PDT 2012


Hi Andrew,

I moved includes ErrorHandling.h, DynamicLibrary.h and config.h to the top of file and still only includes linked with StatSymbols.
Attached move_getPointerToNamedFunction-05.patch moved all includes to the top of file.


Regards,
Danil


________________________________
From: Kaylor, Andrew [andrew.kaylor at intel.com]
Sent: Wednesday, March 28, 2012 12:28 PM
To: Danil Malyshev; llvm-commits at cs.uiuc.edu; Jim Grosbach
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

The patch still shows ‘include’ statements in the middle of the JITMemoryManager.cpp file.  Maybe you didn’t regenerate the patch after making those changes?

Otherwise, it looks good.

-Andy

From: Danil Malyshev [mailto:dmalyshev at accesssoftek.com]
Sent: Tuesday, March 27, 2012 6:01 PM
To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu; Jim Grosbach
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hi Andrew,

> There are some ugly issues with ownership of the JITMemoryManager pointer.  The EngineBuilder documentation says that the
> execution engine takes ownership of the pointer upon successful create, but it seems that in the case of the legacy JIT code it
> delegates that ownership to the JITEmitter.  That was fine in the old implementation because the JIT class didn’t keep a copy of
> the pointer.  With your new implementation the situation is complicated by the fact that both the JIT and the JITEmitter have
> copies of the pointer.

> I feel like this code needs some clean-up in the near future anyway to disentangle the memory management from the symbol
> resolution.  For now maybe it is sufficient to add comments in the JIT code to make the ownership of the JMM pointer absolutely clear.

Well, It's looks not so good as I thought before start doing these changes.
The JCE is a JITCodeEmitter and its hasn't any members linked with JMM, so the comments in the JIT code about JMM is a ownership of the JCE looks a bit strange. For the same reason I cannot use something like JCE->getEmitterMemoryManager() in JIT instead of keep copy of JMM pointer.


> Also, I’m confused by the placement of the StatSymbols class and related declarations.  It’s showing up between
> the comment block for the ‘getPointerToNamedFunction() implementation’ and the actual implementation.  I wasn’t
> sure if this was a cut-and-paste error or if you meant to do that because that code was related to getPointerToNamedFunction.

The StatSymbols is a part of getPointerToNamedFunction implementation and used only for it.
It's was in Intercept.cpp before, but the DefaultJITMemoryManager was declared in the anonymous namespace, so it's implementation should be in the same file and Intercept.cpp cannot used for DefaultJITMemoryManager members implementation.

>  In any event, I think at least the ‘include’ statements need to be moved to the top of the file and the function comments should be kept together.

I did it.

Please find attached the patch with these changes. I changed some comments of getPointerToNamedFunction also.

Regards,
Danil


From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
Sent: Monday, March 26, 2012 1:17 PM
To: Danil Malyshev; llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Jim Grosbach
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hi Danil,

There are some ugly issues with ownership of the JITMemoryManager pointer.  The EngineBuilder documentation says that the execution engine takes ownership of the pointer upon successful create, but it seems that in the case of the legacy JIT code it delegates that ownership to the JITEmitter.  That was fine in the old implementation because the JIT class didn’t keep a copy of the pointer.  With your new implementation the situation is complicated by the fact that both the JIT and the JITEmitter have copies of the pointer.

I feel like this code needs some clean-up in the near future anyway to disentangle the memory management from the symbol resolution.  For now maybe it is sufficient to add comments in the JIT code to make the ownership of the JMM pointer absolutely clear.

Also, I’m confused by the placement of the StatSymbols class and related declarations.  It’s showing up between the comment block for the ‘getPointerToNamedFunction() implementation’ and the actual implementation.  I wasn’t sure if this was a cut-and-paste error or if you meant to do that because that code was related to getPointerToNamedFunction.  In any event, I think at least the ‘include’ statements need to be moved to the top of the file and the function comments should be kept together.

Otherwise, I think your changes look good, and I think they could be committed after minor cleanup for the issues described above.

-Andy


From: Danil Malyshev [mailto:dmalyshev at accesssoftek.com]
Sent: Saturday, March 24, 2012 7:32 PM
To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Jim Grosbach
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hi Andrew,


I agree, also because the JMM can be added to the JIT only in the constructor, so if a NULL will used instead of value, it's can not be changed.
Please review attached the patch.


Regards,
Danil


________________________________
From: Kaylor, Andrew [mailto:andrew.kaylor at intel.com]
Sent: Friday, March 23, 2012 4:36 AM
To: Danil Malyshev; llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Jim Grosbach
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hi Danil,

It isn’t entirely clear to me what the old expected behavior was, but the fact that you had to update the tests seems like a red flag.

There are likely other users of this code who are creating JIT execution engines without creating a memory manager.  I think it would be better to have the JIT constructor create a default memory manager if NULL is passed in for that argument.

The check for a null JMM before use in getPointerToNamed function is good, but the function will still likely fail in many cases if JMM is null.

-Andy

From: Danil Malyshev [mailto:dmalyshev at accesssoftek.com]
Sent: Thursday, March 22, 2012 3:56 PM
To: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>; Jim Grosbach; Kaylor, Andrew
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hello everyone,


The move_getPointerToNamedFunction-01.patch has causing failures in the LazyLoadedJITTest.MaterializableAvailableExternallyFunctionIsntCompiled and MultiJitTest.JitPool unit tests.
This tests uses JIT.getPointerToNamedFunctions() for JIT with NULL memory manager, so call JMM->getPointerToNamedFunctions() raise exception.
This patch move the implementation of getPointerToNamedFunctions from JIT to defaultJMM, so I think, the best way is change this tests to use JIT with default memory manager.
Please review attached move_getPointerToNamedFunction-02.patch with these changes.
For easily looking I also attached the changes.diff. It's contains only changes linked with the problem.

Tested in Ubuntu 64 and Mac OS 64.


Regards,
Danil


________________________________
From: Danil Malyshev
Sent: Wednesday, March 21, 2012 10:41 PM
To: 'Jim Grosbach'
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: RE: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

Hello,


Committed at r153205, thank you.



Regards,
Danil

________________________________
From: Jim Grosbach [mailto:grosbach at apple.com]
Sent: Wednesday, March 21, 2012 1:46 AM
To: Danil Malyshev
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
Subject: Re: [llvm-commits] Move getPointerToNamedFunction() from JIT/MCJIT to JITMemoryManager.

LGTM. Thanks!
On Mar 20, 2012, at 11:48 AM, Danil Malyshev <dmalyshev at accesssoftek.com<mailto:dmalyshev at accesssoftek.com>> wrote:

Hello everyone,

Based on this discussion: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120305/138477.html
Please find attached the patch for review.
This patch doing following:
1. Declares a virtual function getPointerToNamedFunction() in JITMemoryManager
2. Moves the implementation of getPointerToNamedFunction() form JIT/MCJIT to DefaultJITMemoryManager.


Regards,
Danil
<move_getPointerToNamedFunction-01.patch>_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/e3ca1d25/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: move_getPointerToNamedFunction-05.patch
Type: application/octet-stream
Size: 17206 bytes
Desc: move_getPointerToNamedFunction-05.patch
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120328/e3ca1d25/attachment.obj>


More information about the llvm-commits mailing list