#3315 Loop fusion of builtins on the same any_space_

[mo-alistairp]

When multiple builtins in an algorithm are on the same any_space_ they should iterate over the same number of DoFs. Hence, these are candidates for fusion. This is currently missed in the LFRic loop fusion validation step and can be extended to allow this increased functionality. This change should allow the removal of some PSyKAl-lite code in LFRic.

[mo-alistairp]

To note, I've changed test_loop_fuse_error to instead check for correct fusion. This does simultaneously cause a reduction in test coverage in cases where there are builtins on any_space_ but these are different spaces (i.e., the TransformationError on lines 188-192). I'll need to add that coverage too

[mo-alistairp]

So the failure is occurring when PSycloning tutorial/training/lfric/4.2-openmp/solution/main_alg.x90 with transformation script tutorial/training/lfric/4.2-openmp/solution/openmp.py with the error originating from validate in src/psyclone/psyir/transformations/parallel_region_trans.py. When fusing the two setval_c builtins the second node gets detached, as expected, however that node still appears in the all_node list. So when trying to then add OMPParallelTrans we get an error because the parent node of the second node has been removed. This problem (and the subsequent ones) can be subverted by:

• removing the try/except block around the fuse;
• only trying to fuse if len(all_loops)>1
• adding try/except blocks to the OMP transformations to avoid the detached node.

However, I'm not sure if this is the behaviour we actually want from the fuse functionality. Leaving this dead node around seems unhelpful but it seems plausible that it could just be that the transformation script isn't well written for this case.

[mo-alistairp]

Also, when checking the invoke.view() I was on the fence about whether these should be fused at all since one is on W0 and the other W3. However, they're both assigned the same any_space_ so I believe they should have the same number of DoFs.

[arporter]

Also, when checking the invoke.view() I was on the fence about whether these should be fused at all since one is on W0 and the other W3. However, they're both assigned the same any_space_ so I believe they should have the same number of DoFs.

I think they probably shouldn't be fused then - the "same any_space_..." argument only applies if they are arguments to the same kernel. If they are to different kernels, even in the same invoke, there's no guarantee that the spaces are the same.

[arporter]

transformation script tutorial/training/lfric/4.2-openmp/solution/openmp.py with the error originating from validate in src/psyclone/psyir/transformations/parallel_region_trans.py. When fusing the two setval_c builtins the second node gets detached, as expected, however that node still appears in the all_node list. So when trying to then add OMPParallelTrans we get an error because the parent node of the second node has been removed. This problem (and the subsequent ones) can be subverted by:

So previously we refused to fuse so this wasn't a problem? I agree the script is not well written - after the fusing, the all_loops list needs to be recomputed because, as you say, one or more loops will have disappeared.

[mo-alistairp]

transformation script tutorial/training/lfric/4.2-openmp/solution/openmp.py with the error originating from validate in src/psyclone/psyir/transformations/parallel_region_trans.py. When fusing the two setval_c builtins the second node gets detached, as expected, however that node still appears in the all_node list. So when trying to then add OMPParallelTrans we get an error because the parent node of the second node has been removed. This problem (and the subsequent ones) can be subverted by:

So previously we refused to fuse so this wasn't a problem? I agree the script is not well written - after the fusing, the all_loops list needs to be recomputed because, as you say, one or more loops will have disappeared.

Yes, so previously the loop fusion would fail so you wouldn't lose the node in all_loops.

[mo-alistairp]

Also, when checking the invoke.view() I was on the fence about whether these should be fused at all since one is on W0 and the other W3. However, they're both assigned the same any_space_ so I believe they should have the same number of DoFs.

I think they probably shouldn't be fused then - the "same any_space_..." argument only applies if they are arguments to the same kernel. If they are to different kernels, even in the same invoke, there's no guarantee that the spaces are the same.

I don't think my brain has caught up from being off, so apologies for the dumb question! Can I double check the functionality that we're trying to implement here?

If I'm understanding what you said correctly then this should fall into that category because the next invoke is call invoke( name = 'summation', summation_w0_to_w3_kernel_type(field_3, field_0) ) where they are seemingly both arguments in the same invoke

[christophermaynard]

I have had a discussion with @mo-alistairp and I believe the problem with fusing two fields in an invoke that only use builtins is PSyclone has no way of knowing what space they are on. However, whilst this may have arisen in the training material, would this ever arise in LFRic? The only way to interact with a field is via an invoke. So it could only arise via flow control e.g. if a compute z=x/y or if b compute z=x for example but then they would need to be more complex chains of builtin i.e. more than one. However, the condition would be on a scalar, (so maybe min max). It would probably be possible to re-write the alg to avoid doing this - if we ever do. I think we don't. If we don't allow this we will miss most of the fusion we want to do. I think we can fuse in this way. We can solve this with a run-time check on undf?

[arporter]

Just to see if I've understood you @christophermaynard, I think you're saying that there won't be any (or very few) cases of an invoke where the same field is passed to multiple builtins? I can see that that is probably true. A grep of an application working directory would help here but I can't currently get at a machine where I've build LFRic recently.

Therefore, I think you're saying that if we find candidates for fusion, we should just check their undf and branch in the generated code accordingly? That would work. It would be even better (simpler generated code) to see whether we can determine whether two different fields are actually on the same space by static analysis (looking at any other, user-supplied kernels they are passed to in an invoke). However, in terms of this specific task, the check on undf would accomplish the same thing.

[christophermaynard]

Hi @arporter I am saying that there will be no instances of invokes with only builtins, so the assignment of any_space for builtins is sufficient. There is no reason to have only builtin invokes. The only place it could conceivably occur is in the transport where the could be branching on a scalar and either side of the branch, a different calculation - a different invoke would occur. I don't believe they would have multiple builtins though and if they did, transport is entirely W2 for wind so they are all the same space.
So the space assigned to a field in every invoke can either be derived from a kernel with full metadata, or in rare instances a field may be derived from a builtin only, but the other field involved will be derived from a kernel and these fields will be necessarily on the same space.