(Towards #2381 #2674) add min max access types and new LFRic builtins

[arporter]

This is a step towards both #2381 (generalising global reductions) and #2674 (new field_min_max builtin). It attempts to reduce the size of #3222 by only adding the two new access types for reductions as well as the skeletons for the new builtins.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.91%. Comparing base (470e0f7) to head (7a0d563).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3263   +/-   ##
=======================================
  Coverage   99.91%   99.91%           
=======================================
  Files         376      376           
  Lines       53522    53555   +33     
=======================================
+ Hits        53477    53510   +33     
  Misses         45       45           

☔ 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.

[arporter]

Ready for a first look from anyone still working @sergisiso or @hiker ?
It shouldn't affect the IT but I'll fire them off to be on the safe side.

[sergisiso]

@arporter Most of the PR looks good but I am getting confused by the need of these specific access types, these seem regular writes to me. It would be good to clarify if these are really needed before continuing.

[sergisiso]

tab to spaces :)

[arporter]

Ah! Done.

[sergisiso]

I don't understand why do we need GH_SUM, GH_MIN or GH_MAX. These is the output scalar of the built-in which will only be written to, Why not GH_WRITE?

[arporter]

This is a good point. Currently, we rely on knowing the type of reduction being performed by a built-in when creating the Invoke (in lfric_invoke.py):

At this point, we don't have the PSyIR for the built-in - just its name and metadata. Therefore, if the scalar argument was just GH_WRITE we would know that it was a reduction but could not determine the type. I feel we may have conflated DSL metadata "access" information with PSyIR access concepts.

[sergisiso]

Again MIN/MAX is not an AccessType (same with the pre-existing SUM). It is even more confusing that this refer to the output scalar, which is just written to. So why do we differentiate them?

[arporter]

As commented elsewhere, I agree. I'll look to see if I can restrict the MIN/MAX/SUM concept to the kernel metadata.

[arporter]

I thought the best way to solve this would be to have class LFRicAccessType(AccessType) which would add the API-specific access types. Unfortunately, one cannot extend an Enum (https://stackoverflow.com/questions/33679930/how-to-extend-python-enum) so this simply doesn't work.

[sergisiso]

At this point, we don't have the PSyIR for the built-in - just its name and metadata. Therefore, if the scalar argument was just GH_WRITE we would know that it was a reduction but could not determine the type

Could we not just determine the type using its name?

I thought the best way to solve this would be to have class LFRicAccessType(AccessType) which would add the API-specific access types.

I am not sure if this would be just moving the problem, I don't feel it is information that belong to the AccessType. Looking at the code snippet that you shared is the reason for not being GH_WRITE the validation? So we don't allow scalar GH_WRITE because they would be a race condition, but we allow scalar reduction writes because the synchronisation is built-in in the operation? In this case, could we add a GH_SAFE_WRITE/GH_ATOMIC_WRITE, this could be the metadata of the reduction built-ins but also for kernels where the race condition is handled inside the kernel (although we may not want to advertise this feature).

This would be useful in the generic AccessType because in the future we may want to identify these safe_writes in generic code as they have less dependencies that what we give them now.

[arporter]

My feeling is that we currently have quite a lot of domain-specific information in AccessType which should not be there (e.g. INC). All of the generic PSyIR should, as you say, use READ/WRITE/READWRITE. The concept of a reduction or INC access belongs with a domain. I was therefore planning on separating the two out and ensuring that the domain-specific access types are only referred to in appropriate places in the code.

I'm not that keen on the idea of having a mapping of particular built-ins to the type of reduction they perform - it feels more natural to keep that information as part of the metadata associated with the kernel.

The potential future re-use of ATOMIC_WRITE is attractive but I'd be concerned about letting that out into the wild, even unadvertised (we could have it as an internal access type but not as kernel metadata perhaps?).

[arporter]

LFRicConstants already has e.g. VALID_ACCESS_TYPES:

[arporter]

Having now realised that we also have AccessType.INC and AccessType.READINC, it's going to be no small matter to attempt to unpick this (see e.g. #3272). I therefore propose that we keep things as they are for the moment and have a more in-depth discussion about whether we want to tackle this.

[arporter]

@sergisiso ready for another look and/or continued discussion.