From f5b3980ed5a0436fedb870ab8641175fac41454b Mon Sep 17 00:00:00 2001 From: Balthasar Reuter Date: Tue, 11 Jun 2024 21:56:59 +0200 Subject: [PATCH 1/2] Fix misclassification as StatementFunction (#326) --- loki/expression/tests/test_expression.py | 71 ++++++++++++++++++++++++ loki/frontend/fparser.py | 39 ++++++++----- 2 files changed, 95 insertions(+), 15 deletions(-) diff --git a/loki/expression/tests/test_expression.py b/loki/expression/tests/test_expression.py index 1c16f651f..b55cd935a 100644 --- a/loki/expression/tests/test_expression.py +++ b/loki/expression/tests/test_expression.py @@ -2197,3 +2197,74 @@ def static_func(a): test_str = 'foo%bar%barbar%barbarbar%val_barbarbar + 1' parsed = parse_expr(convert_to_case(f'{test_str}', mode=case), evaluate=True, context=context) assert parsed == 6 + + +@pytest.mark.parametrize('frontend', available_frontends( + skip={OMNI: "OMNI fails on missing module"} +)) +def test_stmt_func_heuristic(frontend, tmp_path): + """ + Our Fparser/OFP translation has a heuristic to detect statement function declarations, + but that falsely misinterpreted some assignments as statement functions due to + missing shape information (reported in #326) + """ + fcode = """ +SUBROUTINE SOME_ROUTINE(YDFIELDS,YDMODEL,YDCST) +USE FIELDS_MOD , ONLY : FIELDS +USE TYPE_MODEL , ONLY : MODEL +USE VAR_MOD , ONLY : ARR, FNAME +IMPLICIT NONE +TYPE(FIELDS) ,INTENT(INOUT) :: YDFIELDS +TYPE(MODEL) ,INTENT(IN) :: YDMODEL +TYPE(TOMCST) ,INTENT(IN) :: YDCST +CHARACTER(LEN=20) :: CLFILE +REAL :: ZALFA +REAL :: ZALFAG(3) +REAL :: FOEALFA +REAL :: PTARE +FOEALFA(PTARE) = MIN(1.0, PTARE) +#include "fcttre.func.h" + +ASSOCIATE(YDSURF=>YDFIELDS%YRSURF,RTT=>YDCST%RTT) +ASSOCIATE(SD_VN=>YDSURF%SD_VN,YSD_VN=>YDSURF%YSD_VN, & + & LEGBRAD=>YDMODEL%YRML_PHY_EC%YREPHY%LEGBRAD) +IF(LEGBRAD)SD_VN(:,YSD_VN%YACCPR5%MP,:)=SD_VN(:,YSD_VN%YACCPR%MP,:) +IF(LEGBRAD)ARR(:,YSD_VN%YACCPR5%MP,:)=SD_VN(:,YSD_VN%YACCPR%MP,:) +CLFILE(1:20)=FNAME +ZALFA=FOEDELTA(RTT) +ZALFAG(1)=FOEDELTA(RTT-1.) +ZALFAG(2)=FOEALFA(RTT) +ZALFAG(3)=FOEALFA(RTT-1.) +END ASSOCIATE +END ASSOCIATE +END SUBROUTINE SOME_ROUTINE + """.strip() + source = Sourcefile.from_source(fcode, frontend=frontend, xmods=[tmp_path]) + routine = source['some_routine'] + + assignments = FindNodes(ir.Assignment).visit(routine.body) + + assert [ + ass.lhs.name.lower() for ass in assignments + ] == [ + 'sd_vn', 'arr', 'clfile', 'zalfa', 'zalfag', 'zalfag', 'zalfag' + ] + + sd_vn = assignments[0].lhs + assert isinstance(sd_vn, sym.Array) + + arr = assignments[1].lhs + assert isinstance(arr, sym.Array) + assert arr.type.imported + + # FOEDELTA cannot be identified as a statement function due to the declarations + # hidden in the external header + assert isinstance(assignments[3].rhs, sym.Array) + assert isinstance(assignments[4].rhs, sym.Array) + + # FOEALFA should have been identified as a statement function + stmt_funcs = FindNodes(ir.StatementFunction).visit(routine.ir) + assert len(stmt_funcs) == 1 + assert stmt_funcs[0].name.lower() == 'foealfa' + assert isinstance(assignments[5].rhs, sym.InlineCall) + assert isinstance(assignments[6].rhs, sym.InlineCall) diff --git a/loki/frontend/fparser.py b/loki/frontend/fparser.py index 65729ed00..57a0f2b02 100644 --- a/loki/frontend/fparser.py +++ b/loki/frontend/fparser.py @@ -2983,22 +2983,31 @@ def visit_Assignment_Stmt(self, o, **kwargs): # Special-case: Identify statement functions using our internal symbol table symbol_attrs = kwargs['scope'].symbol_attrs - if isinstance(lhs, sym.Array) and not lhs.parent and lhs.name in symbol_attrs: - - def _create_stmt_func_type(stmt_func): - name = str(stmt_func.variable) - procedure = LazyNodeLookup( - anchor=kwargs['scope'], - query=lambda x: [ - f for f in FindNodes(ir.StatementFunction).visit(x.spec) if f.variable == name - ][0] - ) - proc_type = ProcedureType(is_function=True, procedure=procedure, name=name) - return SymbolAttributes(dtype=proc_type, is_stmt_func=True) + if isinstance(lhs, sym.Array) and symbol_attrs.lookup(lhs.name) is not None: + # If this looks like an array but we have an explicit scalar declaration then + # this might in fact be a statement function. + # To avoid the costly lookup for declarations on each array assignment, we run through + # some sanity checks instead that allow us to bail out early in most cases + lhs_type = lhs.type + could_be_a_statement_func = not ( + lhs_type.shape or lhs_type.length # Declaration with length or dimensions + or lhs.parent # Derived type member (we might lack information from enrichment) + or lhs_type.intent or lhs_type.imported # Dummy argument or imported from module + or isinstance(lhs.scope, ir.Associate) # Symbol stems from an associate + ) + + if could_be_a_statement_func: + def _create_stmt_func_type(stmt_func): + name = str(stmt_func.variable) + procedure = LazyNodeLookup( + anchor=kwargs['scope'], + query=lambda x: [ + f for f in FindNodes(ir.StatementFunction).visit(x.spec) if f.variable == name + ][0] + ) + proc_type = ProcedureType(is_function=True, procedure=procedure, name=name) + return SymbolAttributes(dtype=proc_type, is_stmt_func=True) - if not lhs.type.shape and not lhs.type.intent: - # If the LHS array access is actually declared as a scalar, - # we are actually dealing with a statement function! f_symbol = sym.ProcedureSymbol(name=lhs.name, scope=kwargs['scope']) stmt_func = ir.StatementFunction( variable=f_symbol, arguments=lhs.dimensions, From 3526b9fbf7e30ce727ec3227ffc5ba6a8ad2c976 Mon Sep 17 00:00:00 2001 From: Balthasar Reuter Date: Thu, 13 Jun 2024 11:52:38 +0200 Subject: [PATCH 2/2] OFP: improved type and prefix handling --- loki/expression/symbols.py | 6 ++ loki/frontend/ofp.py | 82 ++++++++++++++++--- loki/tests/test_types.py | 4 +- .../build_system/tests/test_dependency.py | 6 +- loki/transformations/tests/test_inline.py | 3 + .../tests/test_transform_derived_types.py | 2 +- 6 files changed, 82 insertions(+), 21 deletions(-) diff --git a/loki/expression/symbols.py b/loki/expression/symbols.py index 5270bdc31..6b9f153f9 100644 --- a/loki/expression/symbols.py +++ b/loki/expression/symbols.py @@ -627,6 +627,12 @@ def __getinitargs__(self): def init_arg_names(self): return self.symbol.init_arg_names + def _lookup_type(self, scope): + """ + Helper method to look-up type information in any :data:`scope` + """ + return self.symbol._lookup_type(scope) + def clone(self, **kwargs): """ Replicate the object with the provided overrides. diff --git a/loki/frontend/ofp.py b/loki/frontend/ofp.py index 13cd69f30..cb404209b 100644 --- a/loki/frontend/ofp.py +++ b/loki/frontend/ofp.py @@ -27,7 +27,7 @@ from loki import ir from loki.ir import ( GenericVisitor, attach_pragmas, process_dimension_pragmas, - detach_pragmas, pragmas_attached + detach_pragmas, pragmas_attached, FindNodes ) import loki.expression.symbols as sym from loki.expression.operations import ( @@ -37,6 +37,7 @@ from loki.expression import ExpressionDimensionsMapper, AttachScopesMapper from loki.tools import ( as_tuple, disk_cached, flatten, gettempdir, filehash, CaseInsensitiveDict, + LazyNodeLookup ) from loki.logging import debug, info, warning, error from loki.types import BasicType, DerivedType, ProcedureType, SymbolAttributes @@ -457,6 +458,45 @@ def visit_where_stmt(self, o, **kwargs): def visit_assignment(self, o, **kwargs): lhs = self.visit(o.find('target'), **kwargs) rhs = self.visit(o.find('value'), **kwargs) + + # Special-case: Identify statement functions using our internal symbol table + symbol_attrs = kwargs['scope'].symbol_attrs + if isinstance(lhs, sym.Array) and symbol_attrs.lookup(lhs.name) is not None: + # If this looks like an array but we have an explicit scalar declaration then + # this might in fact be a statement function. + # To avoid the costly lookup for declarations on each array assignment, we run through + # some sanity checks instead that allow us to bail out early in most cases + lhs_type = lhs.type + could_be_a_statement_func = not ( + lhs_type.shape or lhs_type.length # Declaration with length or dimensions + or lhs.parent # Derived type member (we might lack information from enrichment) + or lhs_type.intent or lhs_type.imported # Dummy argument or imported from module + or isinstance(lhs.scope, ir.Associate) # Symbol stems from an associate + ) + + if could_be_a_statement_func: + def _create_stmt_func_type(stmt_func): + name = str(stmt_func.variable) + procedure = LazyNodeLookup( + anchor=kwargs['scope'], + query=lambda x: [ + f for f in FindNodes(ir.StatementFunction).visit(x.spec) if f.variable == name + ][0] + ) + proc_type = ProcedureType(is_function=True, procedure=procedure, name=name) + return SymbolAttributes(dtype=proc_type, is_stmt_func=True) + + f_symbol = sym.ProcedureSymbol(name=lhs.name, scope=kwargs['scope']) + stmt_func = ir.StatementFunction( + variable=f_symbol, arguments=lhs.dimensions, + rhs=rhs, return_type=symbol_attrs[lhs.name], + label=kwargs.get('label'), source=kwargs.get('source') + ) + + # Update the type in the local scope and return stmt func node + symbol_attrs[str(stmt_func.variable)] = _create_stmt_func_type(stmt_func) + return stmt_func + return ir.Assignment(lhs=lhs, rhs=rhs, label=kwargs['label'], source=kwargs['source']) def visit_pointer_assignment(self, o, **kwargs): @@ -943,19 +983,19 @@ def visit_declaration(self, o, **kwargs): length = None kind = None - if tk1 in ('', 'len'): + if tk1.lower() in ('', 'len'): # The first child _should_ be the length selector length = self.visit(o[0], **kwargs) - if tk2 == 'kind' or selector_idx > 2: + if tk2.lower() == 'kind' or selector_idx > 2: # There is another value, presumably the kind specifier, which # should be right before the char-selector kind = self.visit(o[selector_idx-1], **kwargs) - elif tk1 == 'kind': + elif tk1.lower() == 'kind': # The first child _should_ be the kind selector kind = self.visit(o[0], **kwargs) - if tk2 == 'len': + if tk2.lower() == 'len': # The second child should then be the length selector assert selector_idx > 2 length = self.visit(o[1], **kwargs) @@ -1125,7 +1165,7 @@ def visit_defined_operator(self, o, **kwargs): name = f'OPERATOR({o.attrib["definedOp"]})' return sym.Variable(name=name) - def _create_Subroutine_object(self, o, scope): + def _create_Subroutine_object(self, o, scope, prefix=None): """Helper method to instantiate a Subroutine object""" from loki.subroutine import Subroutine # pylint: disable=import-outside-toplevel,cyclic-import assert o.tag in ('subroutine', 'function') @@ -1165,7 +1205,12 @@ def _create_Subroutine_object(self, o, scope): if suffix.attrib['result'] == 'result': result_name = header_ast.find('name').attrib['name'] - prefix = [a.attrib['spec'].upper() for a in header_ast.findall('t-prefix-spec')] or None + if prefix: + prefix = [a.attrib['spec'].upper() for a in prefix if a.tag == 't-prefix-spec'] + else: + prefix = [] + if header_ast: + prefix += [a.attrib['spec'].upper() for a in header_ast.findall('t-prefix-spec')] if routine is None: routine = Subroutine( @@ -1276,10 +1321,14 @@ def visit_module(self, o, **kwargs): # subroutine objects using the weakref pointers stored in the symbol table. # I know, it's not pretty but alternatively we could hand down this array as part of # kwargs but that feels like carrying around a lot of bulk, too. - contains = [ - self._create_Subroutine_object(member_ast, kwargs['scope']) - for member_ast in contains_ast if member_ast.tag in ('subroutine', 'function') - ] + contains = [] + prefix = [] + for member_ast in contains_ast: + if member_ast.tag in ('subroutine', 'function'): + contains += [self._create_Subroutine_object(member_ast, kwargs['scope'], prefix)] + prefix = [] + else: + prefix += [member_ast] # Parse the spec spec = self.visit(spec_ast, **kwargs) @@ -1567,10 +1616,14 @@ def visit_names(self, o, **kwargs): return tuple(self.visit(c, **kwargs) for c in o.findall('name')) def visit_name(self, o, **kwargs): + scope = kwargs.get('scope', None) if o.find('generic-name-list-part') is not None: # From an external-stmt or use-stmt - return sym.Variable(name=o.attrib['id']) + name = o.attrib['id'] + if scope: + scope = scope.get_symbol_scope(name) + return sym.Variable(name=name, scope=scope) if o.find('generic_spec') is not None: return self.visit(o.find('generic_spec'), **kwargs) @@ -1582,6 +1635,10 @@ def visit_name(self, o, **kwargs): name, parent = self.visit(part_ref, **kwargs), name if parent: name = name.clone(name=f'{parent.name}%{name.name}', parent=parent) + scope = parent.scope + if scope: + scope = scope.get_symbol_scope(name.name) + name = name.clone(scope=scope) if part_ref.attrib['hasSectionSubscriptList'] == 'true': if i < num_part_ref - 1 or o.attrib['type'] == 'variable': @@ -1776,7 +1833,6 @@ def create_typedef_procedure_declaration(self, comps, iface=None, attrs=None, sc symbols = tuple(s.rescope(scope=scope) for s in symbols) return ir.ProcedureDeclaration(symbols=symbols, interface=iface, source=source) - def create_typedef_variable_declaration(self, t, comps, attr=None, scope=None, source=None): """ Utility method to create individual declarations from a group of AST nodes. diff --git a/loki/tests/test_types.py b/loki/tests/test_types.py index 4f70388ec..b7424f58e 100644 --- a/loki/tests/test_types.py +++ b/loki/tests/test_types.py @@ -94,9 +94,7 @@ def test_symbol_attributes_compare(): assert not someint.compare(somereal) -@pytest.mark.parametrize('frontend', available_frontends(xfail=[ - (OFP, 'OFP needs preprocessing to support contiguous keyword' -)])) +@pytest.mark.parametrize('frontend', available_frontends()) def test_type_declaration_attributes(frontend): """ Test recognition of different declaration attributes. diff --git a/loki/transformations/build_system/tests/test_dependency.py b/loki/transformations/build_system/tests/test_dependency.py index 7034e0dd8..d29a73092 100644 --- a/loki/transformations/build_system/tests/test_dependency.py +++ b/loki/transformations/build_system/tests/test_dependency.py @@ -514,8 +514,7 @@ def test_dependency_transformation_inline_call(frontend): assert 'kernel_test' in [str(s) for s in imports[0].symbols] -@pytest.mark.parametrize('frontend', available_frontends( - xfail=[(OFP, 'OFP does not correctly handle result variable declaration.')])) +@pytest.mark.parametrize('frontend', available_frontends()) def test_dependency_transformation_inline_call_result_var(frontend): """ Test injection of suffixed kernel, accessed through inline function call. @@ -675,8 +674,7 @@ def test_dependency_transformation_contained_member(frontend, use_scheduler, tmp assert calls[0].name == 'get_b' -@pytest.mark.parametrize('frontend', available_frontends( - xfail=[(OFP, 'OFP does not correctly handle result variable declaration.')])) +@pytest.mark.parametrize('frontend', available_frontends()) def test_dependency_transformation_item_filter(frontend, tmp_path, config): """ Test that injection is not applied to modules that have no procedures diff --git a/loki/transformations/tests/test_inline.py b/loki/transformations/tests/test_inline.py index 7c1507ae9..8958176d2 100644 --- a/loki/transformations/tests/test_inline.py +++ b/loki/transformations/tests/test_inline.py @@ -87,6 +87,9 @@ def test_transform_inline_elemental_functions(here, builder, frontend): routine = Subroutine.from_source(fcode, definitions=module, frontend=frontend) inline_elemental_functions(routine) + # Make sure there are no more inline calls in the routine body + assert not FindInlineCalls().visit(routine.body) + # Verify correct scope of inlined elements assert all(v.scope is routine for v in FindVariables().visit(routine.body)) diff --git a/loki/transformations/tests/test_transform_derived_types.py b/loki/transformations/tests/test_transform_derived_types.py index 63cb2e469..00fa6b68f 100644 --- a/loki/transformations/tests/test_transform_derived_types.py +++ b/loki/transformations/tests/test_transform_derived_types.py @@ -1048,7 +1048,7 @@ def test_transform_derived_type_arguments_optional_named_arg(frontend): assert calls[2].kwarguments == (('opt1', '1'), ('val', '1'), ('t_arr', 't%arr'), ('opt2', '2')) -@pytest.mark.parametrize('frontend', available_frontends(xfail=[(OFP, 'No support for recursive prefix')])) +@pytest.mark.parametrize('frontend', available_frontends()) def test_transform_derived_type_arguments_recursive(frontend): fcode = """ module some_mod