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

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

[sergisiso]

@arporter Ok regarding leaving the separation to a follow up PR (#3272) but ...

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 type of reduction they perform belongs to the implementation of the built-in, and you already added it there:
LFRicMinvalXKern.lower_to_languate_level has:

  minval = IntrinsicCall.create(IntrinsicCall.Intrinsic.MIN,
                                [lhs.copy(), arg_refs[0]])

LFRicMaxvalXKern.lower_to_languate_level has:

  minval = IntrinsicCall.create(IntrinsicCall.Intrinsic.MAX,
                                [lhs.copy(), arg_refs[0]])

LFRicSumXKern.lower_to_languate_level has:

  rhs = BinaryOperation.create(BinaryOperation.Operator.ADD,
                               lhs.copy(), arg_refs[0])

My point is that we don't need to repeat this information again in the access type, because the access type is the same for all of them. Would calling the metadata GH_REDUCTION and the AccessType.REDUCTION instead of GH_SUM, GH_MIN, GH_MAX and SUM,MIN,MAX be alright?

[arporter]

My point is that we don't need to repeat this information again in the access type, because the access type is the same for all of them. Would calling the metadata GH_REDUCTION and the AccessType.REDUCTION instead of GH_SUM, GH_MIN, GH_MAX and SUM,MIN,MAX be alright?

That's a good idea. Fortunately, since only built-ins can do reductions this change wouldn't affect any existing LFRic code (I think). I can try doing that and see how it goes.

[arporter]

Currently blocked by #3274 which is moving the access mapping out of the config file.