-
Notifications
You must be signed in to change notification settings - Fork 13
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
Draft: Proposal: resolution of the concrete procedure #340
base: main
Are you sure you want to change the base?
Conversation
…eric function is called
…en a generic interface is used
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.
First of all, a huge apology for letting this hang for so long! This is a really cool extension of the type system and neatly implemented.
I left a few comments throughout with a suggestion how to resolve the problem that you were hitting and described in a comment. Let me know what you think.
Lastly, due to the long time before I took a proper look at this, we have now a few conflicts with the main branch. These will need to be resolved after merging main or rebasing over main. Sorry about that.
@@ -426,6 +426,43 @@ def map_inline_do(self, expr, *args, **kwargs): | |||
return self.rec(expr.bounds, *args, **kwargs) | |||
|
|||
|
|||
class ExpressionTypeMapper(Mapper): |
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.
This is very neat!
A small naming suggestion, since we implicitly make that distinction in the type system: I would call this ExpressionDatatypeMapper
because "type" is what we use for SymbolAttributes
, which include Datatype (dtype
) as well as other declaration attributes.
# Figure out the exact procedure being called if this is a call to a generic interface | ||
if name.type.dtype.is_generic: | ||
# If the interface is imported, take its definition from a module | ||
# TODO: handle interfaces defined in the same module | ||
if name.type.imported: | ||
module = name.type.module | ||
interface = [i for i in module.interfaces if i.spec.name == name.name] | ||
if len(interface) == 1: | ||
interface = interface[0] | ||
# Generic interface contains an abstract function definition and concrete function implementations. | ||
# We need to get rid of the abstract definition. | ||
concrete_symbols = [symbol for symbol in interface.symbols if not symbol.type.dtype.is_generic] | ||
|
||
expr_type_mapper = ExpressionTypeMapper() | ||
expr_dim_mapper = ExpressionDimensionsMapper() | ||
|
||
passed_arguments_types = [(expr_type_mapper(arg), expr_dim_mapper(arg)) for arg in arguments] | ||
|
||
# Try match passed arguments with one of the concrete functions from the interface | ||
for symbol in concrete_symbols: | ||
parameters = symbol.type.dtype.parameters | ||
declared_parameters_types = [(expr_type_mapper(param), expr_dim_mapper(param)) for param in parameters] | ||
# Find matching concrete function | ||
if declared_parameters_types == passed_arguments_types: | ||
ptype: ProcedureType = name.type.dtype | ||
new_procedure_type = ProcedureType(name=name.name, | ||
is_function=ptype.is_function, | ||
is_generic=ptype.is_generic, | ||
return_type=ptype.return_type, | ||
concrete_procedure=symbol.type.dtype.procedure) | ||
new_symbol_attribute = SymbolAttributes(dtype=new_procedure_type, | ||
imported=name.type.imported, | ||
module=name.type.module) | ||
# When the scope is name.scope, then the concrete_procedure takes always the same value ( | ||
# bug or my incomprehension?) | ||
name = sym.ProcedureSymbol(name=name.name, scope=None, type=new_symbol_attribute) |
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.
I'm not convinced that attaching this information to the procedure type will work as expected. The reason is, that all symbols in a scope with the same name share the same entry in the symbol table. Therefore, your example of an int and real variant of the same generic function will create a conflict.
I think the above logic will have to be moved into the CallStatement
node, either into the procedure_type
or routine
property:
Lines 1039 to 1073 in b93a01d
def procedure_type(self): | |
""" | |
The :any:`ProcedureType` of the :any:`Subroutine` object of the called routine | |
For a :class:`CallStatement` node called ``call``, this is shorthand for ``call.name.type.dtype``. | |
If the procedure type object has been linked up with the corresponding | |
:any:`Subroutine` object, then it is available via ``call.procedure_type.procedure``. | |
Returns | |
------- | |
:any:`ProcedureType` or :any:`BasicType.DEFERRED` | |
The type of the called procedure. If the symbol type of the called routine | |
has not been identified correctly, this may yield :any:`BasicType.DEFERRED`. | |
""" | |
return self.name.type.dtype | |
@property | |
def routine(self): | |
""" | |
The :any:`Subroutine` object of the called routine | |
Shorthand for ``call.name.type.dtype.procedure`` | |
Returns | |
------- | |
:any:`Subroutine` or :any:`BasicType.DEFERRED` | |
If the :any:`ProcedureType` object of the :any:`ProcedureSymbol` | |
in :attr:`name` is linked up to the target routine, this returns | |
the corresponding :any:`Subroutine` object, otherwise `None`. | |
""" | |
procedure_type = self.procedure_type | |
if procedure_type is BasicType.DEFERRED: | |
return BasicType.DEFERRED | |
return procedure_type.procedure |
This has also other benefits:
- The procedure chasing is performed only when required - making the frontend faster in all other situations.
- The property is dynamic, which ensure this stays up-to-date when other IR transformations change the arguments. For example, we apply variable promotion/demotion, which might cause a different implementation of a generic function to become the correct procedure.
- The feature is not tied to a single frontend but becomes available to others, too.
@@ -2066,3 +2066,57 @@ def test_import_of_private_symbols(here, frontend): | |||
assert var.type.imported is True | |||
# Check if the symbol comes from the mod_public module | |||
assert var.type.module is mod_public | |||
|
|||
|
|||
@pytest.mark.parametrize('frontend', [FP]) |
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.
Please parameterize tests always with available_frontends()
. If a test cannot be made to work with one of the frontends, that test realization should be explicitly skipped/xfailed instead.
mod_swap = Module.from_source(code_swap_module, frontend=frontend) | ||
mod_main = Module.from_source(code_main_module, frontend=frontend, definitions=[mod_swap]) |
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.
Please add the tmp_path
fixture to the test and use that as xmods
directory here, to avoid that OMNI litters the current working directory with its xmod files.
mod_swap = Module.from_source(code_swap_module, frontend=frontend) | |
mod_main = Module.from_source(code_main_module, frontend=frontend, definitions=[mod_swap]) | |
mod_swap = Module.from_source(code_swap_module, frontend=frontend, xmods=[tmp_path]) | |
mod_main = Module.from_source(code_main_module, frontend=frontend, definitions=[mod_swap], xmods=[tmp_path]) |
assert calls[0].procedure_type.concrete_procedure == procedure_symbols[1] # swap_real | ||
assert calls[1].procedure_type.concrete_procedure == procedure_symbols[0] # swap_int |
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.
Related to the above remark about moving the resolution to the CallStatement
node, this should then be modified. Also, I think we might want to use the stronger is
comparison here.
assert calls[0].procedure_type.concrete_procedure == procedure_symbols[1] # swap_real | |
assert calls[1].procedure_type.concrete_procedure == procedure_symbols[0] # swap_int | |
assert calls[0].routine is mod_swap['swap_real'] | |
assert calls[1].routine is mod_swap['swap_int'] |
interface = [i for i in module.interfaces if i.spec.name == name.name] | ||
if len(interface) == 1: | ||
interface = interface[0] |
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.
You should be able to shortcut this with
interface = [i for i in module.interfaces if i.spec.name == name.name] | |
if len(interface) == 1: | |
interface = interface[0] | |
interface = module.interface_map[name.name] |
# When the scope is name.scope, then the concrete_procedure takes always the same value ( | ||
# bug or my incomprehension?) |
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.
This is precisely the problem that I'm describing. Hence my alternative suggestion which ensures that you can attach the correct scope to the symbol. We will enforce a strict requirement for having a symbol scope in the future, hence this workaround would no longer work.
This is a proposal draft implementation of resolution of the concrete procedure used behind a call to a generic interface. More in: #339