-
Notifications
You must be signed in to change notification settings - Fork 147
Refactored How Calling in CompiledSDFG Works
#2206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 4 commits
2b8123a
767260d
f901a3d
e138b06
b09c9fc
b029828
ded5df8
c2c1116
7f17e13
899b2a0
ab110d2
e8d909e
9397a23
c1214fa
6e1a9ff
69960ce
4da0c4e
2ddabbd
daf90e9
65725f9
41902c3
c069546
68ffa3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |
| import re | ||
| import shutil | ||
| import subprocess | ||
| from typing import Any, Callable, Dict, List, Tuple, Optional, Type, Union | ||
| from typing import Any, Callable, Dict, List, Tuple, Optional, Type, Union, Sequence | ||
| import warnings | ||
| import tempfile | ||
| import pickle | ||
|
|
@@ -331,11 +331,8 @@ def initialize(self, *args, **kwargs): | |
| if self._initialized: | ||
| return | ||
|
|
||
| if len(args) > 0 and self.argnames is not None: | ||
| kwargs.update({aname: arg for aname, arg in zip(self.argnames, args)}) | ||
|
|
||
| # Construct arguments in the exported C function order | ||
| _, initargtuple = self._construct_args(kwargs) | ||
| _, initargtuple = self.construct_arguments(*args, **kwargs) | ||
| self._initialize(initargtuple) | ||
| return self._libhandle | ||
|
|
||
|
|
@@ -376,17 +373,8 @@ def __call__(self, *args, **kwargs): | |
| return the same objects. To force the allocation of new memory | ||
| you can call ``clear_return_values()`` in advance. | ||
| """ | ||
| if self.argnames is None and len(args) != 0: | ||
| raise KeyError(f"Passed positional arguments to an SDFG that does not accept them.") | ||
| elif len(args) > 0 and self.argnames is not None: | ||
| kwargs.update( | ||
| # `_construct_args` will handle all of its arguments as kwargs. | ||
| { | ||
| aname: arg | ||
| for aname, arg in zip(self.argnames, args) | ||
| }) | ||
| argtuple, initargtuple = self._construct_args(kwargs) # Missing arguments will be detected here. | ||
| # Return values are cached in `self._lastargs`. | ||
| argtuple, initargtuple = self.construct_arguments(*args, **kwargs) # Missing arguments will be detected here. | ||
| return self.fast_call(argtuple, initargtuple, do_gpu_check=True) | ||
|
|
||
| def safe_call(self, *args, **kwargs): | ||
|
|
@@ -444,24 +432,22 @@ def safe_call(self, *args, **kwargs): | |
|
|
||
| def fast_call( | ||
| self, | ||
| callargs: Tuple[Any, ...], | ||
| initargs: Tuple[Any, ...], | ||
| callargs: Sequence[Any], | ||
| initargs: Sequence[Any], | ||
| do_gpu_check: bool = False, | ||
| ) -> Union[Tuple[Any, ...], Any]: | ||
| """ | ||
| Calls the underlying binary functions directly and bypassing | ||
| argument sanitation. | ||
| Calls the underlying binary functions directly and bypassing argument sanitation. | ||
|
|
||
| This is a faster, but less user friendly version of ``__call__()``. | ||
| While ``__call__()`` will transforms its Python arguments such that | ||
| they can be forwarded, this function assumes that this processing | ||
| was already done by the user. | ||
| To build the argument vectors you should use `self.construct_arguments()`. | ||
|
|
||
| :param callargs: Arguments passed to the actual computation. | ||
| :param initargs: Arguments passed to the initialization function. | ||
| :param do_gpu_check: Check if errors happened on the GPU. | ||
|
|
||
| :note: You may use `_construct_args()` to generate the processed arguments. | ||
| """ | ||
| from dace.codegen import common # Circular import | ||
| try: | ||
|
|
@@ -498,18 +484,31 @@ def __del__(self): | |
| self._libhandle = ctypes.c_void_p(0) | ||
| self._lib.unload() | ||
|
|
||
| def _construct_args(self, kwargs) -> Tuple[Tuple[Any], Tuple[Any]]: | ||
| """ | ||
| Main function that controls argument construction for calling | ||
| the C prototype of the SDFG. | ||
| def construct_arguments(self, *args: Any, **kwargs: Any) -> Tuple[Tuple[Any], Tuple[Any]]: | ||
| """Construct the argument vectors suitable for from its argument. | ||
|
|
||
| Organizes arguments first by ``sdfg.arglist``, then data descriptors | ||
| by alphabetical order, then symbols by alphabetical order. | ||
| The returned argument vectors are suitable to be passed to `fast_call()`. | ||
| While the function will allocate space for the return values which might | ||
| involve a reallocation. Note that the function will also update the | ||
| internal argument vector cache, i.e. `self._lastargs`. | ||
|
|
||
| :note: If not initialized this function will initialize the memory for | ||
| the return values, however, it might also reallocate said memory. | ||
| :note: This function will also update the internal argument cache. | ||
| :note: The update of `self._lastargs` should be considered a bug rather than a feature. | ||
| """ | ||
| if self.argnames is None and len(args) != 0: | ||
| raise KeyError(f"Passed positional arguments to an SDFG that does not accept them.") | ||
| elif len(args) > 0 and self.argnames is not None: | ||
| positional_arguments = {aname: avalue for aname, avalue in zip(self.argnames, args)} | ||
| if not positional_arguments.keys().isdisjoint(kwargs.keys()): | ||
| raise ValueError( | ||
| f'The arguments where passed once as positional and named arguments: {set(positional_arguments.keys()).intersection(kwargs.keys())}' | ||
| ) | ||
| kwargs.update(positional_arguments) | ||
|
|
||
| # Allocate space for the return value. | ||
| # NOTE: Calling this function is the reason why we have to update `self._lastargs`, | ||
| # because, if the return values are reallocated the memory of them might get | ||
| # reclaimed and the pointers stored in `self._lastargs` are dangling. | ||
| # TODO: Find a better solution that makes this function have no side effects. | ||
| self._initialize_return_values(kwargs) | ||
|
|
||
| # Add the return values to the arguments, since they are part of the C signature. | ||
|
|
@@ -539,26 +538,24 @@ def _construct_args(self, kwargs) -> Tuple[Tuple[Any], Tuple[Any]]: | |
| argnames = [] | ||
| sig = [] | ||
|
|
||
| # Type checking | ||
| cargs = [] | ||
| no_view_arguments = not Config.get_bool('compiler', 'allow_view_arguments') | ||
| for i, (a, arg, atype) in enumerate(zip(argnames, arglist, argtypes)): | ||
| carg = dt.make_ctypes_argument(arg, | ||
| atype, | ||
| a, | ||
| allow_views=not no_view_arguments, | ||
| symbols=kwargs, | ||
| callback_retval_references=self._callback_retval_references) | ||
| cargs.append(carg) | ||
|
|
||
| cargs = tuple( | ||
| dt.make_ctypes_argument(aval, | ||
| atype, | ||
| aname, | ||
| allow_views=not no_view_arguments, | ||
| symbols=kwargs, | ||
| callback_retval_references=self._callback_retval_references) | ||
| for aval, atype, aname in zip(arglist, argtypes, argnames)) | ||
| constants = self.sdfg.constants | ||
|
||
| symbols = self._free_symbols | ||
| callparams = tuple((carg, aname) for arg, carg, aname in zip(arglist, cargs, argnames) | ||
| if not (symbolic.issymbolic(arg) and (hasattr(arg, 'name') and arg.name in constants))) | ||
| if not ((hasattr(arg, 'name') and arg.name in constants) and symbolic.issymbolic(arg))) | ||
|
|
||
| newargs = tuple(carg for carg, aname in callparams) | ||
| newargs = tuple(carg for carg, _aname in callparams) | ||
| initargs = tuple(carg for carg, aname in callparams if aname in symbols) | ||
|
|
||
| # See note above why we _have_ update them. | ||
philip-paul-mueller marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| self._lastargs = newargs, initargs | ||
| return self._lastargs | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? It's not a bug, it caches the result types to avoid reallocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion a call to
construct_arguments()should not update theself._lastargsbecause at that point it has not been called this only happens infast_call()(which should for speed reasons also not do it).The place where this update should happen is inside
__call__(), to be exact betweenconstruct_arguments()andfast_call().And to be honest here, I would remove
_lastargsNow is the reason why this is not possible.
construct_arguments()must call_initialize_return_values(), because the return value is part of the argument vectors.Now there are a few very obscure points:
_lastargsonly contains the pointer to the memory, thus keep them is not enough for keeping the memory allocated._initialize_return_values()does not always allocate new memory, if the memory still has the right size it will not allocate them anew (you can force a reallocation by callingclear_return_value()).This has the nice side effect that the return values might be shared, I use the word might here, because depending on the situation they are or they are not (BTW: That behaviour is not documented).
These two reasons implies that if you reallocate the return value you should throw the pointers away because they might become dangling.
I mean because the user is free to do anything what he wants anyway the input he provided could also become dangling.
Therefore I propose the following changes:
construct_arguments()is called new return values are allocated, there is no sharing._lastargsmight be useful for debugging and would thus propose, to keep it, but to never use it and set it in__call__in all other cases ignore it.