[memory-analysis] Added a field to CPPMethod to store memory-ownershi… - #35
[memory-analysis] Added a field to CPPMethod to store memory-ownershi…#35keremsahn wants to merge 1 commit into
Conversation
Sorry, something went wrong.
| //---------------------------------------------------------------------------- | ||
| // FIXME: For now every allocation is assumed to be done with `new` | ||
| // will be fixed soon. Also the reason function returns an AllocType | ||
| // instead of bool, IsAllocator function from CppInterOp changed a little, |
There was a problem hiding this comment.
You can test these changes on top of your CppInterOp branches by updating the remote and ref here: https://github.com/compiler-research/cppjit/blob/main/CMakeLists.txt#L14-L15
Sorry, something went wrong.
There was a problem hiding this comment.
The changes this PR depends on are already part of main in CppInterOp.
Sorry, something went wrong.
There was a problem hiding this comment.
I meant for changes that have not been merged yet (referred to in the FIXME comment on IsAllocator). iiuc the API on the CppInterOp side will change pending some work being merged, so if you want to coverage test the cppjit patch, this could help
Sorry, something went wrong.
|
I am converting to draft for now. Once I am happy with the state of this PR, I will mark it "ready for review" for others. |
Sorry, something went wrong.
| class memAnalysisKlass { | ||
| int val; | ||
| }; | ||
| __attribute__((malloc)) memAnalysisKlass* allocTest(); | ||
| __attribute__((ownership_returns(malloc))) memAnalysisKlass* allocTestReturns(); |
There was a problem hiding this comment.
Put everything in this file into a namespace; otherwise, the names may conflict with things that are JIT compiled or exist in other files.
Sorry, something went wrong.
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Some minor comments. LGTM, otherwise.
Sorry, something went wrong.
|
|
||
| obj = cppjit.gbl.allocTestReturns() | ||
| assert type(obj) == cppjit.gbl.memAnalysisKlass | ||
| assert obj.__python_owns__ |
There was a problem hiding this comment.
We also need tests for
assert not obj.__python__owns__
Sorry, something went wrong.
| virtual PyObject* GetArgDefault(int /* iarg */, bool silent = true) = 0; | ||
| virtual bool IsConst() { return false; } | ||
| virtual cppjit::interop::AllocType GetAllocBehaviour() { | ||
| return cppjit::interop::AllocType::None; |
There was a problem hiding this comment.
| return cppjit::interop::AllocType::None; | |
| return cppjit::interop::AllocType::Unknown; |
?
Sorry, something went wrong.
There was a problem hiding this comment.
Only class does not override GetAllocBehaviour is TPythonCallBack, and from my understanding this class is for some sort of python function, not C/C++, therefore I thought it would make sense to return None
Sorry, something went wrong.
|
@Vipul-Cariappa I applied Aaron's suggestion, this PR will fetch CppInterOp from my fork's latest branch, so I added bunch of things, worth reviewing again |
Sorry, something went wrong.
…p information and this information effects kIsCreator flag of overload group , currently analyzer is not called, just attribute checker is called
When overload resolution happens and a suitable candidate found and called, HandleReturn helper is called for every case. In this function I call GetAllocBehaviour function for the first element of overload candidates (I followed design where IsCreator flag belongs to overload group, not unique for every PyCallable, but I think of changing it).
@Vipul-Cariappa @aaronj0