[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
| //---------------------------------------------------------------------------- | ||
| // 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
There was a problem hiding this comment.
The changes this PR depends on are already part of main in CppInterOp.
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
|
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. |
| 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.
Vipul-Cariappa
left a comment
Vipul-Cariappa
left a comment
There was a problem hiding this comment.
Some minor comments. LGTM, otherwise.
|
|
||
| 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__| 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; |
?
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
…p information and this information effects kIsCreator flag of overload group , currently analyzer is not called, just attribute checker is called
571a436 to
41ce0d9
Compare
3 participants
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).
2 important notes: Enum order of AllocType is going to change in CppInterOp after open PRs are merged,
and IsAllocator returns bool in the current CppInterOp but this is also going to change after PRs get merged, that is why I keep AllocType in CPPMethod even though it is not useful now
@Vipul-Cariappa @aaronj0