Add basic means to force promote variables to parallel regions

[MetBenjaminWent]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

@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]

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]

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]

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]

@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]

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]

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]

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]

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]

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]

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 ?

[MetBenjaminWent]

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]

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

[LonelyCat124]

@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]

Instead of \ use (), i.e.

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

[MetBenjaminWent]

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

[LonelyCat124]

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?

[MetBenjaminWent]

@LonelyCat124, this should be ready for another review pass! Thanks

[LonelyCat124]

Hi @MetBenjaminWent - one new comment to fix alongside a general formatting preference from our side, when you have code e.g.:

    long_string = (
        "Error: \"Could not find 'j' in the Symbol Table.\" This has been "
        "provided with the 'j' in the 'force_private' option, "
        "but there is no such symbol in this scope.")

(also for function definitions)
we prefer the final bracket on a new line like:

    long_string = (
        "Error: \"Could not find 'j' in the Symbol Table.\" This has been "
        "provided with the 'j' in the 'force_private' option, "
        "but there is no such symbol in this scope."
    )

Can you fix this for your new code?

[LonelyCat124]

remove print statement.

[LonelyCat124]

Since all remaining changes are (essentially formatting) I will set ITs going again in the mean time.