Generalise hoist_arguments_to_temporaries in NEMO utils to all arguments with array operations

[LonelyCat124]

No description provided.

[codecov]

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3429   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         391      391           
  Lines       54683    54689    +6     
=======================================
+ Hits        54664    54670    +6     
  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.

[LonelyCat124]

Some failures after this change can be resolved with a change to reference2arrayrange_trans:

            if not ((isinstance(node.parent, IntrinsicCall) and
                 node.parent.intrinsic in REDUCTION_INTRINSICS)
                or node.parent.is_elemental):
                return

instead of
if not node.parent.is_elemental
However, this still fails for stpctl.f90 for somer aeason, as we end up with an UnresolvedType in the gen_decls part of the backend, which is a bit confusing, I'm trying to track that down.

[LonelyCat124]

I'm trying building today and it just seems to work, I will test full integration and see if any others fail.

[LonelyCat124]

From discussion with sergi the previous direction has potential to produce incorrect code.

Instead we should change operation order:

1. Do the array transformations that don't involve new temporaries
2. Create new temporaries
3. Reapply the array transformations.

[LonelyCat124]

Another option is to move the

• convert_array_notation
• loopify_array_intrinsics
• convert_range_loops
• hoist_argument_expressions
  parts of normalise_loops into a loopifyTrans metatransformation which I wanted to do for while, no individual enabling flags needed.
  Then the DataNodeToTempTrans apply can call this loopifyTrans on the extracted expression as it still have them inside the apply.

We don't have metatransformation support yet for this, but I'll make an issue for this as well.

[LonelyCat124]

@sergisiso I think this is ready for a first look now.

[sergisiso]

Thanks @LonelyCat124, its good to see it has passed all integration tests without making it exclusive for iom_put. Just some extra clarifications and testing needed.

[sergisiso]

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

[LonelyCat124]

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?)

[sergisiso]

I found out in #3343 that node.datatype of a array_reference with a character string is special because those are not array with shapes but ScalarType with length. And therefore its datatype result is wrong. Could this be the same problem?

(If this PR is ready before #3443 I can explore it, e.g. remove the condition, there)

[LonelyCat124]

@sergisiso I think this is ready for review again? I'm not sure why i didn't mark it as so last week so if I've missed something let me know.

[sergisiso]

@LonelyCat124 This is approved for merging, we still don't know what's the issue with characters and why we don't see the expected speedup, but it is a big step to generalise the functionality and it passes all tests

[sergisiso]

@LonelyCat124 The linkspector action is failing, but I will proceed for now and monitor if it gets fixed eventually.