Add basic means to force promote variables to parallel regions by MetBenjaminWent · Pull Request #3431 · stfc/PSyclone

MetBenjaminWent · 2026-05-13T11:02:05Z

Allow users to force promote into parallel sections with force_private, specifically from MaximalOMPParallelRegionTrans, but implemented into ParallelRegionTrans, so all parallel regions can use the functionality, such as when called via OMPParallelTrans

I've been having another play with master PSyclone, and the example recommended by Sergi, works nicely, but unfortunately doesn't quite cover everything currently.

It would seem we need a means to tell the MaximalOMPParallelRegionTrans that we want things to be private.

In bdy_impl3.F90 (I'm just prototyping with this one as its representative of much of the boundary layer, but also shorter), its a spanned parallel section with OMP do(s).

When running it through a script based on this example, we need to ignore the dependencies on these variables for the new ii blocking loops.
(https://github.com/MetBenjaminWent/lfric_apps/blob/4834ffccfc8999b6482a6f2a94accc207a8b58fc/applications/lfric_atm/optimisation/meto-ex1a/transmute/boundary_layer/bdy_impl3.py)

  ignore_dependencies_for =[
           "ct_ctq",
           "dqw",
           "dtl",
           "temp",
           "temp_out",
           "ctctq1",
           "dqw1",
           "dtl1",
           "l"
           ]

However, some arrays like temp or temp_out need to remain private, and the spanned parallel section defaults them to shared.

sergisiso · 2026-05-13T11:15:45Z

Thanks @MetBenjaminWent , I agree we need this (the ability to specify force_private for region transformations now that the region directive supports it and by extension the MaximalOMPRegionTrans). To be ready to merge we will need:

Some example tests in src/psyclone/tests/psyir/transformations/maximal_region_trans_test.py and src/psyclone/tests/psyir/transformations/parallel_region_trans_test.py (each for the corresponding transformation, it can be a code snippet testing it like the currently existing in maximal_region_trans_test.py)
Fix flake8 issues
Convert the print into a log, or if not add a TODO to Add support for logging #11 and we will do it later.
Understand why the force_private was needed everywhere, the kwargs should already propagate the option. Does it not?

MetBenjaminWent · 2026-05-13T12:09:32Z

Thanks @MetBenjaminWent , I agree we need this (the ability to specify force_private for region transformations now that the region directive supports it and by extension the MaximalOMPRegionTrans). To be merge we will need:

Some example tests in src/psyclone/tests/psyir/transformations/maximal_region_trans_test.py and src/psyclone/tests/psyir/transformations/parallel_region_trans_test.py (each for the corresponding transformation, it can be a code snippet testing it like the currently existing in maximal_region_trans_test.py)

Fix flake8 issues

Convert the print into a log, or if not add a TODO to Add support for logging #11 and we will do it later.

Understand why the force_private was needed everywhere, the kwargs should already propagate the option. Does it not?

Thanks Sergi, happy to have a plug away with these, I'm glad it's in the ballpark!

I'm still getting a little used to the kwargs arg, I am hoping to utilise it a little more, it might reduce the need for some of them! I think I had a few issues off the apply in the validate arguments option? Adding it in solved it

LonelyCat124 · 2026-05-13T13:37:07Z

Few comments on what I think we should also have in this PR since this is something we want to address automatically with improvements in the current work packages.

Issue explaining the issue (if its not covered by Improved handling of scalar dependencies in OpenMP #598 ) and xfailing test showing a small example of a missed private variable.

From a code perspective this is a bit more challenging - MaximalRegionTrans is not a great transformation to support this option as we may have MaximalRegionTrans inherited children (e.g. the ProfileRegionTrans in the nemo examples) that do transformations that don't support this option.

Instead it should only be on MaximalOMPParallelRegionTrans. The challenge is how to affect the call to apply in MaximalRegionTrans. The only thing I can think of is to add an transformation_options: dict[str, Any]={} argument before the **kwargs, and then we can do:

par_trans.apply(block, **transformation_options)

instead of the current apply and this can generalise it to any child, and the MaximalOMPParallelRegionTrans can still have force_private = ... and just pass transformation_options={"force_private": force_private} to the super.apply(nodes, ..., **kwargs) call.

@sergisiso Does this seem reasonable to you? The force_private isn't inherited from OMPParallelTrans due to limitations with metatransformations right now - it could be possible but we're not there yet.

The one minor thing is that technically transformation_options could be passed in through **kwargs to MaximalOMPParallelRegionTrans and is inherited, so we'd need to add :param transformation_options: ignored.(but more verbose) probably intoMaximalOMPParallelRegionTrans, or we allow it to be used and don't just override the transformation_options` when calling the superclass.

Note that in general if an option is defined on the superclass you don't need to (and usually shouldn't) define it on the child class, instead use get_option("my_opt", **kwargs) if you need to access it in the child class.

MetBenjaminWent · 2026-05-13T13:45:48Z

Few comments on what I think we should also have in this PR since this is something we want to address automatically with improvements in the current work packages.

Issue explaining the issue (if its not covered by Improved handling of scalar dependencies in OpenMP #598 ) and xfailing test showing a small example of a missed private variable.

From a code perspective this is a bit more challenging - MaximalRegionTrans is not a great transformation to support this option as we may have MaximalRegionTrans inherited children (e.g. the ProfileRegionTrans in the nemo examples) that do transformations that don't support this option.

Instead it should only be on MaximalOMPParallelRegionTrans. The challenge is how to affect the call to apply in MaximalRegionTrans. The only thing I can think of is to add an transformation_options: dict[str, Any]={} argument before the **kwargs, and then we can do:
par_trans.apply(block, **transformation_options)
instead of the current apply and this can generalise it to any child, and the MaximalOMPParallelRegionTrans can still have force_private = ... and just pass transformation_options={"force_private": force_private} to the super.apply(nodes, ..., **kwargs) call.

@sergisiso Does this seem reasonable to you? The force_private isn't inherited from OMPParallelTrans due to limitations with metatransformations right now - it could be possible but we're not there yet.

The one minor thing is that technically transformation_options could be passed in through **kwargs to MaximalOMPParallelRegionTrans and is inherited, so we'd need to add :param transformation_options: ignored.(but more verbose) probably intoMaximalOMPParallelRegionTrans, or we allow it to be used and don't just override the transformation_options` when calling the superclass.

Note that in general if an option is defined on the superclass you don't need to (and usually shouldn't) define it on the child class, instead use get_option("my_opt", **kwargs) if you need to access it in the child class.

Thanks for the insight, happy to open an issue which captures this a little deeper, especially where #598 doesn't quite cover it.
Description also has some expanded context that I'd already sent behind the scenes.

sergisiso · 2026-05-13T14:31:13Z

Instead it should only be on MaximalOMPParallelRegionTrans. The challenge is how to affect the call to apply in MaximalRegionTrans. The only thing I can think of is to add an transformation_options: dict[str, Any]={} argument before the **kwargs, and then we can do:

par_trans.apply(block, **transformation_options)

@sergisiso Does this seem reasonable to you? The force_private isn't inherited from OMPParallelTrans due to limitations with metatransformations right now - it could be possible but we're not there yet.

This is why I was mentioning to propagate kwargs, wouldn't it be enough to do par_trans.apply(block, **kwargs) and trans.validate(current_block + [child], **kwargs) instead of introducing a new dict? (functionallity-wise, I know the docs would not catch it)

LonelyCat124 · 2026-05-13T14:39:59Z

Instead it should only be on MaximalOMPParallelRegionTrans. The challenge is how to affect the call to apply in MaximalRegionTrans. The only thing I can think of is to add an transformation_options: dict[str, Any]={} argument before the **kwargs, and then we can do:

par_trans.apply(block, **transformation_options)

@sergisiso Does this seem reasonable to you? The force_private isn't inherited from OMPParallelTrans due to limitations with metatransformations right now - it could be possible but we're not there yet.

This is why I was mentioning to propagate kwargs, wouldn't it be enough to do par_trans.apply(block, **kwargs) and trans.validate(current_block + [child], **kwargs) instead of introducing a new dict? (functionallity-wise, I know the docs would not catch it)

No, since **kwargs could contain options for a MaximalRegionTrans that arne't valid for par_trans and the validate_options would fail.

sergisiso · 2026-05-13T14:49:39Z

Is it because if kwargs reach the root class without being processed there is an error? e.g. we can not have ignored kwargs?

LonelyCat124 · 2026-05-13T14:52:28Z

Is it because if kwargs reach the root class without being processed there is an error? e.g. we can not have ignored kwargs?

Kinda - we made a decision we wanted to validate input options to make sure that provided options were correct, expected and type checked, so if you did e.g. misspelt_otpion = X that you get an error saying you provided misspelt_otpion which isn't allowed, which also means if you provide an option to a transformation that it doesn't accept (directly or through inheritance) then you also get an error.

sergisiso · 2026-05-13T15:02:14Z

Can we do type(self).get_valid_options() and type(self._transformation).get_valid_options() to decide which way to propagate each kwarg, or error if is not in any of the sets?

sergisiso · 2026-05-13T15:14:53Z

@MetBenjaminWent Maybe we can split the PR into 2, keep this to add force_private to OMPParallelTrans (it needs to be the OMPParallelTrans and not the ParallelRegionTrans because only the OMP has the explicitly_private_symbols set) and add some tests for it.

Then me and @LonelyCat124 can discuss in a separate PR how to best propagate the kwargs to it.

What do you think?

LonelyCat124 · 2026-05-13T15:15:19Z

Can we do type(self).get_valid_options() and type(self._transformation).get_valid_options() to decide which way to propagate each kwarg, or error if is not in any of the sets?

The issue is more when we call par_trans.apply() its own validate method will call validate_options and then fail - I'm not really keen to add code to avoid this option validation as I think in general its a good thing - the other fix here is to make this a metatransformation (i.e. class MaximalOMPParallelTrans(MaximalRegionTrans, OMPParallelTrans)), but the infrastructure isn't really ready to support these still. I think a separate transformation_options dict is the cleanest way I could think of for now since we don't know what else may subclass MaximalRegionTrans.

MetBenjaminWent · 2026-05-13T15:37:20Z

Now that I've caught up on the context, and had a read through each of the 4x classes which inherit ParallelRegionTrans,
OMPParallelTrans, OMPSingleTrans, OMPMasterTrans, ACCParallelTrans, I see what is required.

A dict is certainly one way through and probably the safest.
Alternatively, we could also check through the **kwargs, and for each option we know is safe to pass accross those 4x classes, but may not be utlised by the respective class apply method, we could just pipe them through anyway as they'd fall into kwargs, but not be used. Then if it's not present in the original kwags, we can just redundantly initilise it, matching to how it would be in the calls redundant init. It's less safe, but would move away from the options method. We would also do something similar anyway with the dict but.

MetBenjaminWent · 2026-05-13T15:59:21Z

I think I also need to pipe force_private down manually, otherwise I was running into this issue:

File "hot_psyclone_test/PSyclone/.venv/bin/psyclone", line 42, in <module> main(sys.argv[1:]) File "hot_psyclone_test/PSyclone/src/psyclone/generator.py", line 733, in main code_transformation_mode( File "hot_psyclone_test/PSyclone/src/psyclone/generator.py", line 953, in code_transformation_mode trans_recipe(psyir) File "lfric_apps_wcs/psyclone_bdy_lyr_expanded/lfric_apps/applications/lfric_atm/optimisation/meto-ex1a/transmute/boundary_layer/bdy_impl3.py", line 107, in trans MaximalOMPParallelRegionTrans().apply(routine, force_private=force_private) File "hot_psyclone_test/PSyclone/src/psyclone/psyir/transformations/maximal_omp_parallel_region_trans.py", line 93, in apply super().apply(nodes, **kwargs) File "hot_psyclone_test/PSyclone/src/psyclone/psyir/transformations/maximal_region_trans.py", line 257, in apply self.validate(nodes, **kwargs) File "hot_psyclone_test/PSyclone/src/psyclone/psyir/transformations/maximal_region_trans.py", line 231, in validate self.validate_options(**kwargs) File "hot_psyclone_test/PSyclone/src/psyclone/psyGen.py", line 2512, in validate_options raise ValueError(f"'{type(self).__name__}' received invalid " ValueError: 'MaximalOMPParallelRegionTrans' received invalid options ['force_private']. Valid options are '['node_type_check'].

It doesn't seem to be recognising that the option in the ParallelRegionTrans should be enough. I'm naively guessing its pulling it from RegionTrans?

sergisiso · 2026-05-14T08:31:57Z

@MetBenjaminWent This is the problem that @LonelyCat124 was warning about, essentially kwargs are passed to both, the self transformation in self.validate(...)/validate_options(..) and to the par_trans.apply() and should also be passed to the par_trans.validate(). But self and par_trans support a different set of options, and both with fail if they recieve and option that they don't support, so we cannot supply the same kwards dict to both.

sergisiso · 2026-05-14T08:33:46Z

The issue is more when we call par_trans.apply() its own validate method will call validate_options and then fail - I'm not really keen to add code to avoid this option validation as I think in general its a good thing

@LonelyCat124 I am not proposing to avoid this validation, I am saying something like

self_kwargs = {}
trans_kwargs = {}
for key in kwargs:
    found = False
    if key in type(self).get_valid_options():
        found = True
        self_kwargs[key] = kwargs[key]
    if key in type(self._trans).get_valid_options():
        found = True
        self_kwargs[key] = kwargs[key]
   if not found:
       TransformationError(f"Option '{key}', not supported by {self.name} nor {self._trans.name}")
...
self.validate(**self_kwargs)
...
par_trans.apply(block, **trans_kwargs)

Of course this needs to be done also in the validate so it probably could be a method that splits into both subsets of kwargs, but this is the idea.

PS: In fact it can be a utility method provided in the base Transformation class to be used by any metatransformation, accepting a general kwarg and n-transformations classes and returning n-subsets of kwargs (but we can do that in a separate PR)

MetBenjaminWent · 2026-05-14T08:38:08Z

I've had a thought which works around it, and partly answers how we control whats passed through MaximalRegionTrans.

If we add the new function to ParallelRegionTrans, but then call it via super() from OMPParallelTrans
We have control over the right things calling it, and if they don't use that option, it won't get piped in, unless we allow it to
Then we can allow OMPSingleTrans, OMPMasterTrans, ACCParallelTrans if it is sensible to do so.

We can also do some checks in MaximalRegionTrans if we want for further security?

LonelyCat124 · 2026-05-14T09:32:27Z

The issue is more when we call par_trans.apply() its own validate method will call validate_options and then fail - I'm not really keen to add code to avoid this option validation as I think in general its a good thing

@LonelyCat124 I am not proposing to avoid this validation, I am saying something like
self_kwargs = {}
trans_kwargs = {}
for key in kwargs:
    found = False
    if key in type(self).get_valid_options():
        found = True
        self_kwargs[key] = kwargs[key]
    if key in type(self._trans).get_valid_options():
        found = True
        self_kwargs[key] = kwargs[key]
   if not found:
       TransformationError(f"Option '{key}', not supported by {self.name} nor {self._trans.name}")
...
self.validate(**self_kwargs)
...
par_trans.apply(block, **trans_kwargs)
Of course this needs to be done also in the validate so it probably could be a method that splits into both subsets of kwargs, but this is the idea.

PS: In fact it can be a utility method provided in the base Transformation class to be used by any metatransformation, accepting a general kwarg and n-transformations classes and returning n-subsets of kwargs (but we can do that in a separate PR)

I think if you make a metatransformation it would work anyway (i.e. if you have class MaximalOMPParallelRegionTrans(MaximalRegionTrans, OMPParalllelTrans)) via get_valid_options (if you call it on the MaximalOMPParallelRegionTrans at least). Its the other functionality (e.g. docstrings) that doesn't work yet for metatransformations in a nice way (see #3345). The main other issue is that calling the correct apply/validate/etc. methods is rough for metatransformations and I've not worked out a good solution yet. This might be a reasonable approach but we should work out how we can generalise it, probably something like split_meta_options and use the mro to do it automatically with the relevant type checking all at once.

I don't like the if not found branch though , as it already duplicates checks done by validate_options, but worse - I'd rather if we do that approach we have

if not found:
    self_kwargs.append(key)

and let the self.validate raise the failures (as it can give errors for more than one invalid argument). It also raises the question of (admittedly bad implementations) but if option names are shadowed we might get unexpected non-failing results. This shouldn't happen but its something we should be careful of from a software design standpoint I think.

I have a slight reservation with just automatically making all of the options of self._trans for all MaximalRegionTrans going forward without requiring full testing of all options (which could be a pain), but maybe its just handled by the failure states we have already.

sergisiso · 2026-05-14T10:17:51Z

I think if you make a metatransformation it would work anyway (i.e. if you have class MaximalOMPParallelRegionTrans(MaximalRegionTrans, OMPParalllelTrans)) via get_valid_options

Ah, we had a naming confusion here, I was using metatransformations to mean transformations that call other transformations, not that inherit from multiple transformations. I haven't though about apply/validate MRO (but I don't think this PR should go that far)

I don't like the if not found branch though , as it already duplicates checks done by validate_options

What about:

self_kwargs = dict(kwargs)
trans_kwargs = {}
for key in kwargs:
    if key in type(self._trans).get_valid_options():
        trans_kwargs[key] = kwargs[key]
        if key not in type(self).get_valid_options():
            del self_kwargs[key]

This would only remove self_kwards IFF they are in _trans and not self, so self.validate_options still works as expected for invalid options.

I have a slight reservation with just automatically making all of the options of self._trans for all MaximalRegionTrans going forward without requiring full testing of all options (which could be a pain), but maybe its just handled by the failure states we have already.

I don't think that's true, it does full testing of all options some through
self.validate(self_kwargs)
others with
self._compute_transformable_sections -> trans.validate(current_block + [child], trans_kwargs)

(the trans.validate is missing but I think we need it there otherwise we validate a potentially different thing)

MetBenjaminWent · 2026-05-14T13:02:44Z

Thanks @MetBenjaminWent , I agree we need this (the ability to specify force_private for region transformations now that the region directive supports it and by extension the MaximalOMPRegionTrans). To be ready to merge we will need:

Some example tests in src/psyclone/tests/psyir/transformations/maximal_region_trans_test.py and src/psyclone/tests/psyir/transformations/parallel_region_trans_test.py (each for the corresponding transformation, it can be a code snippet testing it like the currently existing in maximal_region_trans_test.py)

Fix flake8 issues

Convert the print into a log, or if not add a TODO to Add support for logging #11 and we will do it later.

Understand why the force_private was needed everywhere, the kwargs should already propagate the option. Does it not?

@sergisiso, I think relative to work remaining in this PR I just need to look at some new tests now, and this is otherwise ready I believe.
Otherwise #3432 is tightening the arguments down the call tree?

sergisiso · 2026-05-14T14:35:19Z

Can to check if it works if you revert your changes to maximal_region_trans.py and instead merge the new 3432_split_and_propagate_kwargs ?

…ource

…Syclone into force_private_parallel

MetBenjaminWent · 2026-05-18T10:10:42Z

@sergisiso and @LonelyCat124, I think this PR is ready to go, I've added a new test to check that these are added.

I'm not sure where my privileges are with the repo, it looks like I cannot add reviewers, and the checks are stuck pending still?

Thanks!

codecov · 2026-05-19T08:14:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (b45ff81) to head (f8f81d9).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3431   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         391      391           
  Lines       54668    54690   +22     
=======================================
+ Hits        54649    54671   +22     
  Misses         19       19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

MetBenjaminWent · 2026-05-19T09:23:41Z

Re tests, I'm not yet sure how to cover one of the exceptions in: Codecov

I suspect through, after some playing around, the transformation would have already exited by this time, which means it might not be needed at all?

This one though Codecov, I should be able to do something in the parallel region testing

LonelyCat124 · 2026-05-19T09:29:22Z

Re tests, I'm not yet sure how to cover one of the exceptions in: Codecov

I suspect through, after some playing around, the transformation would have already exited by this time, which means it might not be needed at all?

This one though Codecov, I should be able to do something in the parallel region testing

I can't see how the first one could raise a TypeError unless i'm mising something. ancestor would just return None if there's no ancestor, so the next if block would be skipped.

MetBenjaminWent · 2026-05-19T09:53:01Z

Re tests, I'm not yet sure how to cover one of the exceptions in: Codecov
I suspect through, after some playing around, the transformation would have already exited by this time, which means it might not be needed at all?
This one though Codecov, I should be able to do something in the parallel region testing

I can't see how the first one could raise a TypeError unless i'm mising something. ancestor would just return None if there's no ancestor, so the next if block would be skipped.

Ah cool, so if it always returns none, I'll just remove the try!

MetBenjaminWent · 2026-05-19T10:24:58Z

So digging into the second, so explore the try and except of the _check_symbol_table_vars member function call, and test the checking of the symbol table, for whether a variable is present or not.

In this test I've got a simple code block:

    code = """subroutine x
    integer :: i, outside_var
    integer :: array_i(4)
    outside_var = 2
    do i = 1, 4
        array_i(i) = 1 + 2
    end do
    end subroutine x
    """

And I'm choosing to parallelise only the loop nodes. Which therefore outside_var should be always outside the region, and the exception should be called?

Choosing to parallelise over all of the nodes (the assignment then loop), it produces an identical data symbol table, which doesn't seem right.....

I'll push up what I've got so far...

MetBenjaminWent · 2026-05-19T13:55:03Z

So digging into the second, so explore the try and except of the _check_symbol_table_vars member function call, and test the checking of the symbol table, for whether a variable is present or not.
In this test I've got a simple code block:
    code = """subroutine x
    integer :: i, outside_var
    integer :: array_i(4)
    outside_var = 2
    do i = 1, 4
        array_i(i) = 1 + 2
    end do
    end subroutine x
    """
And I'm choosing to parallelise only the loop nodes. Which therefore outside_var should be always outside the region, and the exception should be called?

Choosing to parallelise over all of the nodes (the assignment then loop), it produces an identical data symbol table, which doesn't seem right.....
I'll push up what I've got so far...

So, I think is is working as expected, but I didn't expect this at the time.

The symbol table is always that of the routine, which we get the reference to pass to explicitly_private_symbols in the right format.

It does mean users can put any variables in to be privatised, so long as they are in the routine. But that is also how it works for the parallel loop transformation so.

Regarding the slight loss in testing coverage, I don't think that bit can be tested. I can trip the warning, but I can't capture that warning in a test.

MetBenjaminWent · 2026-05-19T15:59:35Z

This should be ready for review. As noted, I don't think I can test what codecov is upset about.

LonelyCat124 · 2026-05-20T09:13:13Z

@MetBenjaminWent I mocked up an idea on how to test the uncovered code, let me know if you have any issues with it as I haven't tested it properly since I didn't setup an env with your branch yet:

def test_parallelregion_check_symtab_var(fortran_reader, caplog):
    '''
    ...
    '''
    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"))

    assert ("..." in caplog.text)

LonelyCat124

Hi @MetBenjaminWent

Functionality looks good now, but there are code style/formatting issues. I've asked Sergi one other code-style question, which I'll update this PR if we have consensus on but otherwise its just the fixes here.

LonelyCat124 · 2026-05-21T10:54:15Z

@@ -40,6 +40,7 @@
 #          J. Dendy, Met Office


Add yourself to the Author lists on the files modified.

LonelyCat124 · 2026-05-21T10:55:40Z

        '''
        Surrounds the provided node list with an OpenMP Parallel region.

        :param nodes: list of Nodes to put within parallel region.


Document force_private.

(But only on this transformation).

LonelyCat124 · 2026-05-21T10:56:53Z

+        if force_private:
+            new_region_directive = nodes[0].ancestor(RegionDirective)
+            if new_region_directive:
+                region_set = super()._check_symbol_table_vars(


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

LonelyCat124 · 2026-05-21T10:58:27Z

+                         logger="psyclone.psyir.transformations"):
+        otrans._check_symbol_table_vars(parallel, ("j"))
+
+    long_string = \


Instead of \ use (), i.e.

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

LonelyCat124 · 2026-05-21T10:58:44Z

+        "\"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)


Don't need parenthesis on the assert.

LonelyCat124 · 2026-05-21T10:59:49Z

+    assert len(psyir.walk(OMPParallelDirective)) == 1
+    nodes = psyir.walk(Routine)[0].children[:]
+    assert isinstance(nodes[0], OMPParallelDirective) is True
+    pdir = psyir.walk(OMPParallelDirective)


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

LonelyCat124 · 2026-05-21T11:01:02Z

+    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):


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

LonelyCat124 · 2026-05-21T11:02:20Z

+                # 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 "


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.

MetBenjaminWent · 2026-05-21T11:55:40Z

Many thanks Aidan for the CR, I'll get these updated!

LonelyCat124 · 2026-05-21T12:05:40Z

Many thanks Aidan for the CR, I'll get these updated!

@MetBenjaminWent One other thing from discussion from Sergi, we prefer this format:

def func(
    arg1, arg2, arg3, ...
    arg6, arg7, ...
):

for calling or defining functions - can you change that in your code as well?

Add basic means to force promote variables to parallel regions

d6f47ff

Removal of some manual piping to rely on kwags

7a6af64

MetBenjaminWent added 2 commits May 13, 2026 16:06

Further piping change

f8d7273

housekeeping

e7ee06c

MetBenjaminWent added 3 commits May 14, 2026 09:44

Pipe in some variables to enable correct passing validation

e19e40e

formatting fixes, flake8, function data types, logging

900a81f

flake8 format

0e6b185

sergisiso mentioned this pull request May 14, 2026

Split and propagate kwargs into multiple sub-transformations #3432

Closed

MetBenjaminWent added 2 commits May 14, 2026 14:10

Remove snuck through argument which is no longer needed here

67b8682

fix

b0f9d35

MetBenjaminWent and others added 5 commits May 15, 2026 15:21

Fix conflict and remove debugging stuff

fc94fff

Add test for adding first privates to spanned region over difficult s…

1b635a2

…ource

Merge branch 'master' into force_private_parallel

25b03ee

formatting

a000661

Merge branch 'force_private_parallel' of github.com:MetBenjaminWent/P…

64e0387

…Syclone into force_private_parallel

MetBenjaminWent requested review from LonelyCat124 and sergisiso May 19, 2026 09:22

MetBenjaminWent added 3 commits May 19, 2026 11:25

Add changes and debugging findings

a559f9a

Update and accept coverage loss

c467b47

small adjustments

c44ef4a

MetBenjaminWent added 3 commits May 19, 2026 15:01

fix flake8 stuff

4fbdada

update logger

ca0530d

remove old import

7004541

Some housekeeping of comment and unneeded change

a2dfb77

Add test recommended by Aidan

f8f81d9

LonelyCat124 added the under review label May 21, 2026

LonelyCat124 requested changes May 21, 2026

View reviewed changes

LonelyCat124 added reviewed with actions and removed under review labels May 21, 2026

Conversation

MetBenjaminWent commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergisiso commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetBenjaminWent commented May 13, 2026

Uh oh!

LonelyCat124 commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetBenjaminWent commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergisiso commented May 13, 2026

Uh oh!

LonelyCat124 commented May 13, 2026

Uh oh!

sergisiso commented May 13, 2026

Uh oh!

LonelyCat124 commented May 13, 2026

Uh oh!

sergisiso commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergisiso commented May 13, 2026

Uh oh!

LonelyCat124 commented May 13, 2026

Uh oh!

MetBenjaminWent commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetBenjaminWent commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sergisiso commented May 14, 2026

Uh oh!

sergisiso commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetBenjaminWent commented May 14, 2026

Uh oh!

LonelyCat124 commented May 14, 2026

Uh oh!

sergisiso commented May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MetBenjaminWent commented May 14, 2026

Uh oh!

sergisiso commented May 14, 2026

Uh oh!

MetBenjaminWent commented May 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MetBenjaminWent commented May 19, 2026

Uh oh!

LonelyCat124 commented May 19, 2026

Uh oh!

MetBenjaminWent commented May 19, 2026

Uh oh!

MetBenjaminWent commented May 19, 2026

Uh oh!

MetBenjaminWent commented May 19, 2026

Uh oh!

MetBenjaminWent commented May 19, 2026

Uh oh!

LonelyCat124 commented May 20, 2026

Uh oh!

LonelyCat124 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MetBenjaminWent commented May 13, 2026 •

edited

Loading

sergisiso commented May 13, 2026 •

edited

Loading

LonelyCat124 commented May 13, 2026 •

edited

Loading

MetBenjaminWent commented May 13, 2026 •

edited

Loading

sergisiso commented May 13, 2026 •

edited

Loading

MetBenjaminWent commented May 13, 2026 •

edited

Loading

MetBenjaminWent commented May 13, 2026 •

edited

Loading

sergisiso commented May 14, 2026 •

edited

Loading

sergisiso commented May 14, 2026 •

edited

Loading

MetBenjaminWent commented May 18, 2026 •

edited

Loading

codecov Bot commented May 19, 2026 •

edited

Loading