Skip to content
Open
51 changes: 33 additions & 18 deletions examples/nemo/scripts/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
Assignment, Loop, Directive, Node, Reference, CodeBlock, Call,
Routine, Schedule, IntrinsicCall, StructureReference, IfBlock,
Operation)
from psyclone.psyir.symbols import DataSymbol, ArrayType
from psyclone.psyir.symbols import DataSymbol, ArrayType, ScalarType
from psyclone.psyir.transformations import (
ArrayAssignment2LoopsTrans, HoistLoopBoundExprTrans, HoistLocalArraysTrans,
HoistTrans, InlineTrans, Maxval2LoopTrans, Sum2LoopTrans, Minval2LoopTrans,
Expand Down Expand Up @@ -205,10 +205,6 @@ def normalise_loops(
:param hoist_argument_expressions: whether to hoist array expressions
out of the containing Call.
'''
# TODO #3412: This is currently limited to iom_put, we want to expand it
# throughout the code
if hoist_argument_expressions:
iom_put_argument_to_temporary(schedule.walk(Call))

if hoist_local_arrays and schedule.name not in CONTAINS_STMT_FUNCTIONS:
# Apply the HoistLocalArraysTrans when possible, it cannot be applied
Expand Down Expand Up @@ -280,6 +276,20 @@ def normalise_loops(
except TransformationError:
pass

if hoist_argument_expressions:
hoist_arguments_to_temporaries(schedule.walk(Call))
normalise_loops(
schedule,
hoist_local_arrays=hoist_local_arrays,
convert_array_notation=convert_array_notation,
loopify_array_intrinsics=loopify_array_intrinsics,
convert_range_loops=convert_range_loops,
scalarise_loops=scalarise_loops,
increase_array_ranks=False,
hoist_expressions=hoist_expressions,
# Make sure we never repeat this step.
hoist_argument_expressions=False,
)
# TODO #1928: In order to perform better on the GPU, nested loops with two
# sibling inner loops need to be fused or apply loop fission to the
# top level. This would allow the collapse clause to be applied.
Expand Down Expand Up @@ -540,23 +550,28 @@ def _satisfies_minimum_region_rules(self, region: list[Node]) -> bool:
MaximalProfilingOutsideDirectivesTrans().apply(children)


def iom_put_argument_to_temporary(calls: list[Call]):
'''Extracts the second argument of all iom_put calls and puts them
in a temporary if they are an Operation with an array datatype.
def hoist_arguments_to_temporaries(calls: list[Call]):
'''Extracts the arguments of all calls and puts them
in a temporary result if they are an Operation with an array datatype.

:param calls: The list of calls in a subroutine whose arguments
may be moved into temporary storage to allow additional potential
parallelisation.

'''
for call in calls:
if call.symbol.name == "iom_put":
for arg in call.arguments:
dtype = arg.datatype
if (isinstance(dtype, ArrayType) and
(isinstance(arg, Operation) or
isinstance(arg, IntrinsicCall))):
try:
DataNodeToTempTrans().apply(arg, verbose=True)
except TransformationError:
pass
for arg in call.arguments:
dtype = arg.datatype
# Only extract expressions that can potentially be loopfied,
# i.e. operations over arrays.
if (isinstance(dtype, ArrayType) and
(isinstance(arg, Operation) or
isinstance(arg, IntrinsicCall))):
Comment on lines +567 to +569
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a comment explaining the condition: "Only extract expressions that can potentially be loopyfied (e.g. operations over arrays)"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

try:
if (isinstance(dtype.elemental_type, ScalarType)
and dtype.elemental_type.intrinsic ==
ScalarType.Intrinsic.CHARACTER):
continue
Comment on lines +571 to +574
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an issue with the transformation? I am wondering if it should be inside the validation instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure - the transformation does it successfully but there was some error, I don't remember whether compiler or elsewhere in PSyclone. I can have a further look if you want, I just decided to skip it here for strings (not sure if there would be some other valid use case?)

DataNodeToTempTrans().apply(arg, verbose=True)
except TransformationError:
pass
15 changes: 12 additions & 3 deletions src/psyclone/psyir/transformations/datanode_to_temp_trans.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
Loop,
Range,
Reference,
Routine,
Statement,
Schedule,
UnaryOperation,
Expand Down Expand Up @@ -230,7 +231,10 @@ def validate(self, node: DataNode, **kwargs):
f"Statement node which is not supported."
)

if isinstance(dtype, (UnresolvedType, UnsupportedFortranType)):
if (isinstance(dtype, (UnresolvedType, UnsupportedFortranType))
or (isinstance(dtype, ArrayType) and
isinstance(dtype.elemental_type,
(UnresolvedType, UnsupportedFortranType)))):
failing_symbols = []
symbols = node.get_all_accessed_symbols()
for sym in symbols:
Expand Down Expand Up @@ -343,14 +347,19 @@ def apply(self, node: DataNode, storage_name: str = "",
allocatable_datatype.shape])

# Create a symbol of the relevant type.
containing_routine = node.ancestor(Routine)
if containing_routine:
sym_tab = containing_routine.symbol_table
else:
sym_tab = node.scope.symbol_table
if not storage_name:
symbol = node.scope.symbol_table.new_symbol(
symbol = sym_tab.new_symbol(
root_name="tmp",
symbol_type=DataSymbol,
datatype=datatype
)
else:
symbol = node.scope.symbol_table.new_symbol(
symbol = sym_tab.new_symbol(
root_name=storage_name,
symbol_type=DataSymbol,
datatype=datatype
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,6 @@ def validate(self, node, options=None, **kwargs):
raise TransformationError(
f"The dimension argument to {self._INTRINSIC_NAME} is not "
f"yet supported.")

if isinstance(node.datatype, (UnresolvedType, UnsupportedType)):
raise TransformationError(
f"Error in {self.name} transformation. Cannot create "
Expand Down Expand Up @@ -178,6 +177,9 @@ def apply(self, node, options=None, verbose: bool = False, **kwargs):
:param options: options for the transformation.
:type options: Optional[Dict[str, Any]]

:raises TransformationError: if node only contains Reference nodes,
and they can't be convered to ArrayReferences by the calls to
Reference2ArrayRangeTrans.
'''
# TODO 2668: options are now deprecated:
if options:
Expand Down Expand Up @@ -221,6 +223,11 @@ def apply(self, node, options=None, verbose: bool = False, **kwargs):
# resulting in the following code being created:
# a(:,:) = a(:,:)+b(:,:)
array_refs = new_array_expr.walk(ArrayReference)
if len(array_refs) == 0:
raise TransformationError(
f"Can't apply {self.name} to {node.debug_string()} due "
f"to no ArrayReference nodes present."
)
assignment = Assignment.create(array_refs[0].copy(),
new_array_expr.detach())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,6 @@ def apply(self, node, options=None, **kwargs):

'''
self.validate(node, **kwargs)

# The following cases do not need expansions
if node.parent and isinstance(node.parent, Call):
if node is node.parent.routine:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
from psyclone.configuration import Config
from psyclone.psyir.frontend.fortran import FortranReader
from psyclone.psyir.nodes import (
Assignment, Reference
Assignment, Loop, Reference, Routine
)
from psyclone.psyir.symbols import (
DataSymbol, INTEGER_TYPE
Expand Down Expand Up @@ -299,6 +299,9 @@ def test_datanodetotemptrans_apply(fortran_reader, fortran_writer, tmp_path):
psyir = fortran_reader.psyir_from_source(code)
assign = psyir.walk(Assignment)[0]
dtrans.apply(assign.rhs.operands[1])
# Check the tmp symbol is at the routine scope.
routine = psyir.walk(Routine)[0]
assert routine.symbol_table.lookup("tmp") is not None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert "tmp" in routine.symbol_table ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

out = fortran_writer(psyir)
assert """ integer, allocatable, dimension(:,:) :: tmp

Expand All @@ -309,6 +312,27 @@ def test_datanodetotemptrans_apply(fortran_reader, fortran_writer, tmp_path):
d = c + tmp""" in out
assert Compile(tmp_path).string_compiles(out)

# Check the symbol is still created if the assignment is in some other
# non-routine scope.
code = """subroutine test()
integer :: a, b, c
integer :: i

do i = 1, 100
a = b + c
end do
end subroutine test"""
psyir = fortran_reader.psyir_from_source(code)
loop = psyir.walk(Loop)[0].detach()
assign = loop.walk(Assignment)[0]
dtrans.apply(assign.rhs)
assert "tmp" in assign.scope.symbol_table
out = fortran_writer(loop)
assert """do i = 1, 100, 1
tmp = b + c
a = tmp
enddo""" in out

code = """subroutine test()
real :: a
integer :: b
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,3 +141,59 @@ def test_apply(fortran_reader, fortran_writer, tmpdir):
result = fortran_writer(psyir)
assert expected in result
assert Compile(tmpdir).string_compiles(result)


def test_nested_sums_error_cases(fortran_reader):
'''Test that the transformation fails correctly if we have nested sums
as the reference can't be converted to an arrayreference as the reference
is inside a non-elemental function.'''
code = """subroutine sum_test()
integer :: n, m
real , dimension(:, :) :: array
real :: result

result = sum(sum(array, dim=2)) * array(n,m)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the validation counts ArrayReference, maybe test that expressions with other arrays like:
sum(sum(array + array(:,:), dim=2))
or
sum(sum(array, dim=dimensions(2)))

work as expected.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the first one already doesn't work as expected.

    code = """subroutine sum_test()
    integer :: n, m
    real, dimension(:, :) :: array
    result = sum(sum(array  + array(:,:), dim=2))
    end subroutine"""
    psyir = fortran_reader.psyir_from_source(code)
    intrinsic_node = psyir.children[0].children[0].rhs
    trans = Sum2LoopTrans()
    with pytest.raises(TransformationError) as err:
        trans.apply(intrinsic_node)
    print(fortran_writer(psyir))
    print(err.value)
    assert False

This outputs an error message on the fortran_writer stage, and the err.value is also different (if it were reached):

E           psyclone.psyir.backend.visitor.VisitorError: Visitor Error: The following symbols are not explicitly declared or imported from a module and there are no wildcard imports which could be bringing them into scope: 'result'

and

Transformation Error: ArrayAssignment2LoopsTrans could not convert the expression:
array(:,:) = SUM(array(:,:) + array(:,:), 2)

 into a loop because:
Transformation Error: ArrayAssignment2LoopsTrans does not support statements containing dependencies that would generate loop-carried dependencies when naively converting them to a loop, but found:
array(:,:) = SUM(array(:,:) + array(:,:), 2)

I'm not sure if this is the right place to fix this - I can make some xfailing tests and an issue?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sergisiso I've addressed the other comments but this one is still open if you have any time to let me know how to proceed before another full review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both errors seem correct:

  • result is not declared in the example
  • And TransformationErrors are fine, what we don't want is to validate and then generate incorrect code.

So I would say no xfail needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah thats my mistake in the example, I was concerned the resulting code may be changed but I still get the input so thats fine. I'll check the other example you suggested as well.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the other examples - the error messages are different, but the transformation does fail as expected.

end subroutine"""
psyir = fortran_reader.psyir_from_source(code)
intrinsic_node = psyir.children[0].children[0].rhs.children[0]
trans = Sum2LoopTrans()
with pytest.raises(TransformationError) as err:
trans.apply(intrinsic_node)
assert ("Can't apply Sum2LoopTrans to SUM(SUM(array, 2)) due "
"to no ArrayReference nodes present." in str(err.value))

code = """subroutine sum_test()
integer :: n, m
real, dimension(:, :) :: array
real :: result

result = sum(sum(array + array(:,:), dim=2))
end subroutine"""
psyir = fortran_reader.psyir_from_source(code)
intrinsic_node = psyir.children[0].children[0].rhs
trans = Sum2LoopTrans()
with pytest.raises(TransformationError) as err:
trans.apply(intrinsic_node)
assert ("Transformation Error: ArrayAssignment2LoopsTrans does not "
"support statements containing dependencies that would generate "
"loop-carried dependencies when naively converting them to a "
"loop, but found" in str(err.value))

code = """subroutine sum_test()
integer :: n, m
real, dimension(:, :) :: array
integer, dimension(1) :: dimensions
real :: result
result = sum(sum(array, dim=dimensions(2)))
end subroutine"""
psyir = fortran_reader.psyir_from_source(code)
intrinsic_node = psyir.children[0].children[0].rhs
trans = Sum2LoopTrans()
with pytest.raises(TransformationError) as err:
trans.apply(intrinsic_node)
assert ("Transformation Error: Error in ArrayAssignment2LoopsTrans "
"transformation. The LHS of the supplied Assignment node "
"should contain an array accessor with at least one of its "
"dimensions being a Range, but none were found in "
"'dimensions(2) = SUM(array, dimensions(2))"
in str(err.value))
Loading