(Closes #2499) scalarization transformation implementation

[LonelyCat124]

First implementation is here. I have unit tests for the private routines, I need to test it with code examples doing the full transformation properly but in theory its implemented here for now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.89%. Comparing base (06e0257) to head (94f99eb).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2563   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files         359      360    +1     
  Lines       50997    51089   +92     
=======================================
+ Hits        50942    51034   +92     
  Misses         55       55           

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

[LonelyCat124]

@hiker Quick question on how the ordering of statements goes with the dependency system - if you have an assignment such as a = a + 1 does the a access on the RHS appear before the a on the LHS? I assume so (and that makes sense) but I just wanted to double check as I have a superfluous (and unreachable) if statement if so.

[LonelyCat124]

@sergisiso this is ready for a first look.
I didn't add anything to user or dev guide for now, let me know if you think I should.

I'm waiting for hiker to clarify my question before I remove the code at lines 123 in the transformation, but I expect to remove those lines as I think they're unreachable.

[sergisiso]

Good initial implementation @LonelyCat124, I am just asking if the code can be slightly refactored to be more readable and I am wondering if some logic should leave inside the next_access methods, I am happy to discuss this one.

[LonelyCat124]

@sergisiso I've addressed most of the comments here - I think the next_access one is a bigger issue than just here (and we need to decide if the variable access tooling should handle this or if we just do it in next_access, but I think we can do an implementation for this in the mean time anyway).

I need to still expand the tests for the apply routine and add documentation before another review though I think.

[sergisiso]

@LonelyCat124 I don't mind the order of these (if you put the associated TODOs), but wouldn't want to go very far with some logic that then will need to be deleted.

[LonelyCat124]

I brought this up to master now and made the tests pass, but I'll start reworking the logic here now.

[LonelyCat124]

Remaining issues seem to be a bug in DefinitionUseChains. I'm going to try to work this out - once I find it am I ok to fix it here or shall i create an issue and separate pull request @sergisiso ?

[LonelyCat124]

Probable bug: What happens if we find an element of a structure that we think can be scalarised (e.g. mystruct%array(i)) - we shouldn't scalarise? Or we scalarise only if we can work out the type of the element of the structure and then we need to come up with a name. @sergisiso what are your thoughts on how this should be handled?

[LonelyCat124]

NEMO passthrough gives the wrong result - I need to investigate.

[LonelyCat124]

NEMO4 now works, NEMO5 gets the wrong answer, I'll try to investigate next week

[LonelyCat124]

First bug from NEMO5, that i'm not exactly sure how to resolve.

PSyclone scalarizes this first loop:

    if (MOD(kt, nn_trd) == 0 .OR. kt == nit000 .OR. kt == nitend) then
      do jk = 1, jpkm1, 1
        do jj = ntsj - 0, ntej + 0, 1
          do ji = ntsi - 0, ntei + 0, 1
            zkx(ji,jj,jk) = 0._wp
            zky(ji,jj,jk) = 0._wp
            zkz(ji,jj,jk) = 0._wp
            zkepe(ji,jj,jk) = 0._wp
          enddo
        enddo
      enddo
      call eos(ts(:,:,:,:,kmm), rhd, rhop)
      zkz(:,:,1) = 0._w
      do jk = 1, jpk, 1
        do jj = ntsj - 0, ntej + 0, 1
          do ji = ntsi - 0, ntei + 0, 1
            zkz(ji,jj,jk) = xcof * e1e2t(ji,jj) * ww(ji,jj,jk) * (rhop(ji,jj,jk) + rhop(ji,jj,jk - 1)) * tmask_i(ji,jj)
          enddo
        enddo
      enddo

It believes the write to zkz(ji,jj,jk) can be scalarized, because the next access (zkz(:,:,1)) overwrites it with 0. This means the values of zkz(:,:,2:) will be undefined in this example - the next access is also a write and maybe that is why but I need to check.

This might be a common theme and is maybe difficult to resolve correctly. Most of the scalarizations I find in NEMO5 that look bad are this kind of read after read access.

[LonelyCat124]

Ok - replicating this seems to be difficult.
Part of this is because the next_accesses is finding the References inside the LHS object, since the ArrayReference here is not so straightfoward:

Assignment[]
    ArrayReference[name:'arr']
        Range[]
            IntrinsicCall[name='LBOUND']
                Reference[name:'LBOUND']
                Reference[name:'arr']
                Literal[value:'1', Scalar<INTEGER, UNDEFINED>]
            IntrinsicCall[name='UBOUND']
                Reference[name:'UBOUND']
                Reference[name:'arr']
                Literal[value:'1', Scalar<INTEGER, UNDEFINED>]
            Literal[value:'1', Scalar<INTEGER, UNDEFINED>]
        Range[]
            IntrinsicCall[name='LBOUND']
                Reference[name:'LBOUND']
                Reference[name:'arr']
                Literal[value:'2', Scalar<INTEGER, UNDEFINED>]
            IntrinsicCall[name='UBOUND']
                Reference[name:'UBOUND']
                Reference[name:'arr']
                Literal[value:'2', Scalar<INTEGER, UNDEFINED>]
            Literal[value:'1', Scalar<INTEGER, UNDEFINED>]
        Literal[value:'1', Scalar<INTEGER, UNDEFINED>]
    Literal[value:'0.0', Scalar<REAL, UNDEFINED>]

It finds the References to arr inside the IntrinsicCalls. Probably I need to change the DefinitionUseChains to be smarter about enquiry functions (I hate inquiry functions...), we should ignore inquiries I think. I think I should also change Reference.is_read to know about inquiry functions as well.
@sergisiso Are you ok for me to make these changes in this PR or would you prefer a separate PR?