Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d6f47ff
Add basic means to force promote variables to parallel regions
MetBenjaminWent May 13, 2026
7a6af64
Removal of some manual piping to rely on kwags
MetBenjaminWent May 13, 2026
f8d7273
Further piping change
MetBenjaminWent May 13, 2026
e7ee06c
housekeeping
MetBenjaminWent May 13, 2026
e19e40e
Pipe in some variables to enable correct passing validation
MetBenjaminWent May 14, 2026
900a81f
formatting fixes, flake8, function data types, logging
MetBenjaminWent May 14, 2026
0e6b185
flake8 format
MetBenjaminWent May 14, 2026
67b8682
Remove snuck through argument which is no longer needed here
MetBenjaminWent May 14, 2026
b0f9d35
fix
MetBenjaminWent May 14, 2026
f58867f
push up WIP test
MetBenjaminWent May 15, 2026
55b750f
Merge branch '3432_split_and_propagate_kwargs' into force_private_par…
MetBenjaminWent May 15, 2026
5fe3c28
Un pipe force_private
MetBenjaminWent May 15, 2026
14c53ca
Push pipe adjust recomended
MetBenjaminWent May 15, 2026
8e135fa
Merge branch '3432_split_and_propagate_kwargs' of github.com:stfc/PSy…
MetBenjaminWent May 15, 2026
ce47cc4
Merge branch '3432_split_and_propagate_kwargs' into force_private_par…
MetBenjaminWent May 15, 2026
967b357
debugging updates
MetBenjaminWent May 15, 2026
fc94fff
Fix conflict and remove debugging stuff
MetBenjaminWent May 15, 2026
1b635a2
Add test for adding first privates to spanned region over difficult s…
MetBenjaminWent May 18, 2026
25b03ee
Merge branch 'master' into force_private_parallel
MetBenjaminWent May 18, 2026
a000661
formatting
MetBenjaminWent May 18, 2026
64e0387
Merge branch 'force_private_parallel' of github.com:MetBenjaminWent/P…
MetBenjaminWent May 18, 2026
a559f9a
Add changes and debugging findings
MetBenjaminWent May 19, 2026
c467b47
Update and accept coverage loss
MetBenjaminWent May 19, 2026
c44ef4a
small adjustments
MetBenjaminWent May 19, 2026
4fbdada
fix flake8 stuff
MetBenjaminWent May 19, 2026
ca0530d
update logger
MetBenjaminWent May 19, 2026
7004541
remove old import
MetBenjaminWent May 19, 2026
a2dfb77
Some housekeeping of comment and unneeded change
MetBenjaminWent May 20, 2026
f8f81d9
Add test recommended by Aidan
MetBenjaminWent May 21, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/psyclone/psyir/transformations/omp_parallel_trans.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
# J. Dendy, Met Office
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.

Add yourself to the Author lists on the files modified.

'''This module provides the OMPParallelTrans transformation.'''

from collections.abc import Iterable
from psyclone import psyGen
from psyclone.psyir.nodes import (
ACCDirective,
Expand All @@ -48,6 +49,7 @@
OMPParallelDirective,
OMPDirective,
Return,
RegionDirective,
)
from psyclone.psyir.transformations.parallel_region_trans import (
ParallelRegionTrans)
Expand Down Expand Up @@ -130,7 +132,10 @@ def validate(self, nodes: list[Node], options=None, **kwargs):
# TODO #2668: Remove options.
super().validate(nodes, options, **kwargs)

def apply(self, nodes: list[Node], options=None, **kwargs):
def apply(
self, nodes: list[Node],
options=None, force_private: Iterable[str] = (),
**kwargs):
'''
Surrounds the provided node list with an OpenMP Parallel region.

Expand All @@ -139,5 +144,17 @@ def apply(self, nodes: list[Node], options=None, **kwargs):
# TODO #2668: Remove options.
super().apply(nodes, options, **kwargs)

# Privatise the provided variables for the new RegionDirective, if they
# are found within the symbol table of the ancestor Routine.
if force_private:
new_region_directive = nodes[0].ancestor(RegionDirective)
if new_region_directive:
region_set = super()._check_symbol_table_vars(
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.

use self._check_symbol_table_vars, no need to super class this.

new_region_directive,
force_private)
if region_set:
new_region_directive.explicitly_private_symbols.update(
region_set)


__all__ = ["OMPParallelTrans"]
36 changes: 34 additions & 2 deletions src/psyclone/psyir/transformations/parallel_region_trans.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,15 @@
This module provides the implementation of ParallelRegionTrans

'''

import logging
from collections.abc import Iterable
from abc import ABC, abstractmethod
from psyclone.psyir.transformations.transformation_error import (
TransformationError)
from psyclone import psyGen
from psyclone.psyir.transformations.region_trans import RegionTrans
from psyclone.psyir.nodes import CodeBlock, Node, Return
from psyclone.psyir.nodes import CodeBlock, Node, Return, RegionDirective
from psyclone.psyir.symbols import DataSymbol
from psyclone.utils import transformation_documentation_wrapper


Expand All @@ -76,6 +78,36 @@ def __str__(self) -> str:

'''

def _check_symbol_table_vars(
self,
region_node: RegionDirective,
force_private: Iterable[str] = ()) -> set[DataSymbol]:
'''
Check that the symbol table of the provided region node contains the
variable variables in the provided list. Return a set of DataSymbols.

This is intended to be used as part of privatising the variables
contained in the list for the provided region in the child classes.
'''
explicitly_private_symbols = set()

for symbol_name in force_private:
sym = None
try:
sym = region_node.scope.symbol_table.lookup(
symbol_name.lower())
except KeyError as err:
# This is not an error, but we will log the missed string
logger = logging.getLogger(__name__)
logger.warning(
"%s has been provided with the '%s' symbol name in "
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.

Can you use fstring here isntead of formatted strings please? I.e.

f"{err} has been provided with the '{symbol_name}' ..."

Out rules also that each line of the f string must begin f" even if no replacements are in that section of the string.

"the 'force_private' option, but there is no such "
"symbol in this scope.", err, symbol_name)
if sym:
explicitly_private_symbols.add(sym)

return explicitly_private_symbols

def validate(self, nodes: list[Node], options=None, **kwargs):
# pylint: disable=arguments-renamed
'''
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@
Assignment,
IfBlock,
Routine,
Loop,
OMPParallelDirective,
)
from psyclone.psyir.transformations import (
MaximalRegionTrans,
TransformationError,
OMPParallelTrans
OMPParallelTrans,
OMPLoopTrans,
)


Expand Down Expand Up @@ -345,3 +347,43 @@ class OneParTrans(MaximalRegionTrans):
assert isinstance(routine.children[0], OMPParallelDirective)
assert isinstance(routine.children[1], Assignment)
assert isinstance(routine.children[2], OMPParallelDirective)


def test_apply_force_private(fortran_reader):
'''Test the apply function of MaxParallelRegionTrans
with force privates.'''
code = """subroutine x
integer :: i, ii, k, l, block
integer :: array_l(8)
block = 2
do ii = 1, 4
do k = 4, 1, -1
l = 0
do i = ii, min(ii+block -1, 4)
l = l + 1
array_l(l) = 1 + 2
end do
end do
end do
end subroutine x
"""
psyir = fortran_reader.psyir_from_source(code)
# Apply loop_trans to all the loops possible.
ltrans = OMPLoopTrans(omp_schedule="static")
for loop in psyir.walk(Loop):
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.

Any reason for this rather than just ltrans.apply(psyir.walk(Loop)[0]), ...)? This feels tidier to me if its the same functionality.

if loop.variable.name == "ii":
ltrans.apply(
loop,
ignore_dependencies_for=["array_l"],
nowait=True)
# Apply maximum transformation to code
mtrans = MaxParTrans()
routine = psyir.walk(Routine)
# Note, i, ii, k, l seem to be set as first private
mtrans.apply(routine, force_private=["i", "ii", "k", "l", "array_l"])
# assertions
assert len(psyir.walk(OMPParallelDirective)) == 1
nodes = psyir.walk(Routine)[0].children[:]
assert isinstance(nodes[0], OMPParallelDirective) is True
pdir = psyir.walk(OMPParallelDirective)
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.

Just use pdir = nodes[0] since you've asserted where it is already, will be more efficient than a big walk.

assert len(pdir[0].explicitly_private_symbols) == 5
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@

'''

import logging
import pytest
from psyclone.psyir.transformations.transformation_error import (
TransformationError)
from psyclone.psyir.nodes import CodeBlock
from psyclone.psyir.nodes import (Literal, Loop)
from psyclone.psyir.nodes import (CodeBlock, Literal, Loop)
from psyclone.psyir.transformations import OMPParallelTrans
from psyclone.psyir.symbols import (DataSymbol, INTEGER_TYPE)

Expand All @@ -69,3 +69,32 @@ def test_parallelregion_refuse_codeblock():
otrans.validate([parent])
assert ("Nodes of type 'CodeBlock' cannot be enclosed by a "
"OMPParallelTrans transformation" in str(err.value))


def test_parallelregion_check_symtab_var(fortran_reader, caplog):
'''
Check ParallelRegionTrans._check_symbol_table_vars try and except,
if the logging message produces a warning when a variable is not
in the routine scope.We use OMPParallelTrans as ParallelRegionTrans
is abstract.
'''
otrans = OMPParallelTrans()
code = """subroutine test
integer :: i
do i = 1, 100

end do
end subroutine"""
psyir = fortran_reader.psyir_from_source(code)
otrans.apply(psyir.children[0].children[0])
parallel = psyir.children[0].children[0]
caplog.clear()
with caplog.at_level(logging.WARNING,
logger="psyclone.psyir.transformations"):
otrans._check_symbol_table_vars(parallel, ("j"))

long_string = \
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.

Instead of \ use (), i.e.

long_string = (
"Could not find 'j' in the ..."
"provided with the ..."
)

"\"Could not find 'j' in the Symbol Table.\" has been "\
"provided with the 'j' symbol name in the 'force_private' option, "\
"but there is no such symbol in this scope."
assert (long_string in caplog.text)
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.

Don't need parenthesis on the assert.

Loading