Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

quepas
Copy link
Contributor

@quepas quepas commented Jul 3, 2024

This is a proposal draft implementation of resolution of the concrete procedure used behind a call to a generic interface. More in: #339

Copy link
Collaborator

@reuterbal reuterbal left a 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):
Copy link
Collaborator

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.

Comment on lines +2486 to +2521
# 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)
Copy link
Collaborator

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:

loki/loki/ir/nodes.py

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])
Copy link
Collaborator

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.

Comment on lines +2114 to +2115
mod_swap = Module.from_source(code_swap_module, frontend=frontend)
mod_main = Module.from_source(code_main_module, frontend=frontend, definitions=[mod_swap])
Copy link
Collaborator

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.

Suggested change
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])

Comment on lines +2121 to +2122
assert calls[0].procedure_type.concrete_procedure == procedure_symbols[1] # swap_real
assert calls[1].procedure_type.concrete_procedure == procedure_symbols[0] # swap_int
Copy link
Collaborator

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.

Suggested change
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']

Comment on lines +2492 to +2494
interface = [i for i in module.interfaces if i.spec.name == name.name]
if len(interface) == 1:
interface = interface[0]
Copy link
Collaborator

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

Suggested change
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]

Comment on lines +2519 to +2520
# When the scope is name.scope, then the concrete_procedure takes always the same value (
# bug or my incomprehension?)
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants