(Towards #3060) intrinsic return types #3119

LonelyCat124 · 2025-09-02T09:57:02Z

I'm putting this up early so if anyone has time to have a quick look at the implementation before I go too far down the rabbit hole with how I'm implementing the return type and tests.

I have added two more members to the IAttr namedtuple, return_type and reference_accesses, which can either be a Callable or specific value (depending on whats required).

To get the return_type of an IntrinsicCall, my plan is to do something like:

def return_type(self):
    if isinstance(self.intrinsic.return_type, Callable):
        try:
            return self.intrinsic.return_type(self)
         except:
             # The idea here is to handle all of the "bad input cases", e.g. if we have an Unresolved or UnsupportedType in the input
             # I'd rather have a except here than handle it in every single return_type callable.
             return UnresolvedType()
    # If its not a callable we just return the value.
    return self.intrinsic.return_type

The return type implementations are started - there are 3 helper functions at the moment (for cases I expect to be used a lot), e.g. _get_first_argument_type, wheras other's have their own lambda (for example see AINT).

I'm unsure how much to avoid code duplication here, for example AINT and ANINT have the same lambda for their return_type, so I'm not sure whether its worth moving this out (and whether the result should be a lambda or function) every time I have any 2 intrinsic calls with the same return type? Feedback on this specific question would be appreciated as early as possible (probably one for @arporter to answer perhaps).

To test the return types, my plan was to have standalone test for every "helper" function (or even helper lambda later).
I was then planning to create a parametrize test for all other intrinsics who have their own specific lambda. My one concern is this parametrize would become very large - again feedback/thoughts on this approach would be helpful. You can see an initial versoin of how this parametrize might look at intrinsic_call_test::650.

NB. This is dependent on #3110 and I think I will rebase onto that branch for now so I can have passing tests.

LonelyCat124 · 2025-09-02T10:10:39Z

One other coding style question - are we happy with statements like:

    return ScalarType(ScalarType.Intrinsic.REAL,
                      (node.arguments[node.argument_names.index("kind")]
                       if "kind" not in node.argument_names else
                       node.arguments[0].datatype.precision))

Or would you prefer to pull out the if statement? (This was required when this was a lambda, but its turning into a function as its getting a lot of reuse so I'm happy to rewrite it if it is preferred).

LonelyCat124 · 2025-09-02T10:25:19Z

Also one thing to note is PSyclone appears to support more IntrinsicCall than are created by Fparser - e.g. BESSEL_ functions get created as ArrayReferences - I'm not sure if this will be resolved by #3041 .

Also a question for @sergisiso - can you always refer to arguments with their names? I see for example CSHIFT is defined as
RESULT = CSHIFT(ARRAY, SHIFT [, DIM]), but could you do:
RESULT = CSHIFT(shift=3, ARRAY=array)?

LonelyCat124 · 2025-09-02T14:45:00Z

I would also say - there are some cases of reusing precision througout this code. I'm not sure if this is a good idea with the new "precision can be DataNodes" - if not then the review might need to request me to fix that by copying if they're a DataNode. This probably in some cases means some significant rewrites, but I'll wait for the review (I think that datatype.copy might also have this issue?).

LonelyCat124 · 2025-09-02T15:43:40Z

I applied the black formatter to these files as well - I couldn't work out how to make formatting happy myself for a couple of the lambdas so I had to make black do it for me.

arporter · 2025-09-03T08:52:49Z

src/psyclone/psyir/nodes/intrinsic_call.py

+            False,
+            ArgDesc(1, 1, DataNode),
+            {},
+            None,


Strictly speaking, as this is a call to a routine, the type is lambda: NoType(). I was going to say we need to make sure this is consistent with Call.datatype but that method isn't implemented yet :-)

src/psyclone/psyir/nodes/intrinsic_call.py

arporter · 2025-09-03T09:05:17Z

src/psyclone/psyir/nodes/intrinsic_call.py

+    return ArrayType(dtype, new_shape)
+
+
+def _get_first_argument_logical_kind_with_optional_dim(node) -> DataType:


Is it possible to replace the various _get_first_argument_{int,real,logical...}_kind_with... with a version that just takes the intrinsic-type as an argument?

I think I'd need to see an example of how you mean - I'm not sure I have many of these functions that only vary by the Intrinsic type - I think most have some other variation (either have an optional dim or kind or work on scalars vs arrays etc.). Some that take the same named arguments don't behave the same either - some have kind of first argument if no kind whilst others have default kind if no kind.

I did consider a decorator to try to make a single one that was cleaner, but I didn't like the design of that due to the level of disparity between the functions - especially now I've reached some of the more complex return types that really needed to be a function and not a lambda to be readable.

arporter · 2025-09-03T09:15:04Z

One other coding style question - are we happy with statements like:
    return ScalarType(ScalarType.Intrinsic.REAL,
                      (node.arguments[node.argument_names.index("kind")]
                       if "kind" not in node.argument_names else
                       node.arguments[0].datatype.precision)) 
Or would you prefer to pull out the if statement? (This was required when this was a lambda, but its turning into a function as its getting a lot of reuse so I'm happy to rewrite it if it is preferred).

I think I'd prefer a separate if for this - it's quite hard to parse :-)

arporter · 2025-09-03T09:16:22Z

To get the return_type of an IntrinsicCall, my plan is to do something like:

def return_type(self):
    if isinstance(self.intrinsic.return_type, Callable):
        try:
            return self.intrinsic.return_type(self)
         except:
             # The idea here is to handle all of the "bad input cases", e.g. if we have an Unresolved or UnsupportedType in the input
             # I'd rather have a except here than handle it in every single return_type callable.
             return UnresolvedType()
    # If its not a callable we just return the value.
    return self.intrinsic.return_type

I'm a bit confused by the check on whether it is Callable. Could we avoid this by always having a lambda or am I missing something?

arporter · 2025-09-03T09:17:24Z

Thanks Aidan, I think it's looking mostly as I'd expect although, as commented above, I was anticipating always having a Callable - whether a lambda or a separate routine if it's complicated enough.

EDIT: scrub that - I was getting confused between the definition of an IntrinsicCall and an Intrinsic. I think what you're suggesting is fine actually.

LonelyCat124 · 2025-09-03T09:30:55Z

Thanks Aidan, I think it's looking mostly as I'd expect although, as commented above, I was anticipating always having a Callable - whether a lambda or a separate routine if it's complicated enough.

EDIT: scrub that - I was getting confused between the definition of an IntrinsicCall and an Intrinsic. I think what you're suggesting is fine actually.

I could always have a lambda - it just felt overkill for cases where the return type is just an INTEGER_TYPE - I'll finish implementing things as they are now and at review time if the datatype of the IntrinsicCall routine is a bit of a mess we can re-evaluate.

LonelyCat124 · 2025-09-03T14:49:57Z

I will say I semi-lost the will to carry on for specifically THIS_IMAGE - there are no GNU docs for it and I'm not sure our current version is correct anyway so I just gave up and made it UnresolvedType().

I'll clean up the remaining test suite issues before I have "finished" return_type and probably it would be good to have a closer look before I move on to implementing reference_accesses.

codecov · 2025-09-03T15:19:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.91%. Comparing base (97dd02d) to head (4f76d51).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3119    +/-   ##
========================================
  Coverage   99.91%   99.91%            
========================================
  Files         376      376            
  Lines       53529    53675   +146     
========================================
+ Hits        53484    53630   +146     
  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.

LonelyCat124 · 2025-09-03T17:19:54Z

I fixed the remaining failing test and added the keywords for IAttr.
@arporter @sergisiso if either of you have time for a quick look through how I've implemented the return type functiosn and have any (specific) feedback it would be appreciated, as I'll then apply that when I look at reference_accesses as well.
I think we don't need to have a detailed overall review yet (including if the review wants to check I've not made mistakes), as that can wait until later.

LonelyCat124 · 2025-09-04T08:53:04Z

Fixed up the remaining coverage I can do with fortran_reader now - the remaining coverage is uncoveted as PSyclone can only made CodeBlock[StructureAccess] from the inputs.

LonelyCat124 · 2025-09-08T09:10:01Z

@arporter @sergisiso ready for a look now - Andy suggested doing reference accesses as its own PR separately.

There is coverage missing, but PSyclone/fparser doesn't support the inputs that could result in those - I want to leave the correct results in the code for when we do, but I'll leave it to the reviewer to decide.

Edit: Note that this PR incorporates the kind stuff from #3110 - so ignore anything that looks like it comes from changes to kinds.

Edit2: The other thing I'm unsure about for both this PR and the following PR is if we have optional arguments declared on an IntrinsicCall do they HAVE to be named in Fortran? I.e. is only integer(x, kind=wp) legal or can you just do integer(x, wp)?

LonelyCat124 · 2025-09-09T12:02:07Z

Also one note - I think TEAM_IMAGE is a typo/made up intrinsic we have? I think it should be TEAM_NUMBER (https://gcc.gnu.org/onlinedocs/gfortran/TEAM_005fNUMBER.html). If so, then let me know and I'll fix its name at least - I'll leave it to the reviewer as to whether we want return_type (and later reference_Accesses) for it.

LonelyCat124 · 2025-09-11T08:22:43Z

One note - this probably needs a todo w.r.t #2302 - I am rewriting the reference_accesses code to handle that, but this does not handle unexpected naming of arguments (that would cause IntrinsicCall.create to fail).

arporter

I realised that it's a bit early to review this as it's branched off the PR that makes precision a DataNode. Therefore, I've only done a limited look, mainly focused on intrinsic_call.py (once I realised about the branching).
I like the way it's going and thanks for adding the keywords to the arguments to the many IAttr constructors. Mainly it's the usual request for comments plus it would be really helpful to write down the rules that are implemented by the various help methods - if you could do that for all of them (in their docstrings) then that would be great. I think there's also some scope to reduce duplication.

arporter · 2025-09-10T15:23:05Z

examples/psyir/create_structure_types.py


 # Shorthand for a scalar type with REAL_KIND precision
-SCALAR_TYPE = ScalarType(ScalarType.Intrinsic.REAL, REAL_KIND)
+SCALAR_TYPE = ScalarType(ScalarType.Intrinsic.REAL, Reference(REAL_KIND))


I see what you mean about precisions and types now. Maybe SCALAR_TYPE needs to become a routine that returns a new object? Similarly for all the other shorthands we have here.

arporter · 2025-09-10T15:25:37Z

src/psyclone/core/access_sequence.py

+        :raises InternalError: if the variable does not have READ acccess.
+        '''
+        if self._access_type != AccessType.READ:
+            raise InternalError("Trying to change variable to 'TYPE_INFO' "


I can see you've just adapted this from the existing one but I think the text could be better. Perhaps "...change variable access from 'READ' to 'TYPE_INFO' but access type is '{self._access_type}'"

src/psyclone/core/signature.py

arporter · 2025-09-10T15:30:27Z

src/psyclone/domain/common/transformations/kernel_module_inline_trans.py

                # Compare the routine to be inlined with the one that
                # is already present.
                new_rts = self._prepare_code_to_inline([kernel_schedule])
+                print(new_rts[0] == routine, len(new_rts))


arporter · 2025-09-10T15:37:45Z

src/psyclone/tests/domain/common/transformations/kernel_module_inline_trans_test.py

    end module my_mod
    ''')

+    print("----------------------------------------------")


Please rm these prints.

src/psyclone/psyir/nodes/intrinsic_call.py

arporter · 2025-09-11T11:02:54Z

src/psyclone/psyir/nodes/intrinsic_call.py

+    arg2 = Reference(DataSymbol("b", stype2))
+    binop = BinaryOperation.create(BinaryOperation.Operator.MUL,
+                                   arg1, arg2)
+    # TODO - make this a public method?


Probably we do want to make it (BinaryOperation._get_result_scalar_type) a public method.

src/psyclone/psyir/nodes/intrinsic_call.py

arporter · 2025-09-11T12:31:29Z

src/psyclone/psyir/nodes/intrinsic_call.py

+def _maxval_return_type(node) -> DataType:
+    """ Helper function for the MAXVAL (and similar) intrinsic return
+    types.
+


Please document the rules.

arporter · 2025-09-11T12:34:16Z

src/psyclone/psyir/nodes/intrinsic_call.py

+            required_args=ArgDesc(1, 1, DataNode),
+            optional_args={"kind": DataNode},
+            return_type=lambda node: (
+                ScalarType(


I think you may have a utility that does this now?

Still relevant?

Yeah, with an if/else in the choice of kind name this works now definitely.

I think technically I don't, I only have ones that do precision UNDEFINED if not kind, wheras this is specifically the kind of "a".

LonelyCat124 · 2025-12-02T16:15:53Z

Ok, @arporter I think this is finally in a state its ready for a proper look. I've opened #3246 to take a look at the ArrayBounds copy functionality, but the rest here should be fixed from the previous version, and there are fewer functions than before so it should be a bit cleaner.

arporter

Thanks very much Aidan, this is a big piece of work! Mostly minor comments about tidying although the fact that we still catch Exception worries me a bit - see inline.
I'll look at the tests next time.

src/psyclone/core/access_sequence.py

src/psyclone/psyir/nodes/intrinsic_call.py.orig

src/psyclone/psyir/nodes/intrinsic_call.py

arporter · 2025-12-08T16:47:38Z

src/psyclone/psyir/nodes/intrinsic_call.py

+        ) -> DataType:
+    """Helper function for the common IntrinsicCall case where the
+    return type is a Scalar with the kind of a named argument,
+    unless an optional dim named argument exists, in which case an array with


"...optional argument named 'dim' exists, ..."

src/psyclone/psyir/nodes/intrinsic_call.py

LonelyCat124 · 2025-12-11T13:31:31Z

Have a crisis of confidence with the implementation of (what is now) _type_with_specified_kind_and_optional_dim so I need to go back through and check these should be using argument_name.datatype.precision and not argument_name.copy()

…3060_intrinsic_types

LonelyCat124 · 2025-12-12T14:44:15Z

@arporter This is ready for another look now, I fixed the exception thing, now it only catches a specific exception and leaves any other exceptions that I can't see a way to create so they'd be very unexpected.

arporter

Thanks Aidan, that's a lot of new tests!
Functionally, I think it's pretty close now. However, in looking at the tests I thought of some other situations that I don't think we cover yet. Depending on how hard they are to do, this could become a 'towards' PR so we can spread the pain a bit.

arporter · 2025-12-15T16:46:02Z

src/psyclone/tests/core/access_sequence_test.py

-    assert ("Trying to change variable to 'CONSTANT' which does not have "
-            "'READ' access." in str(err.value))
+    assert ("Trying to change variable to 'CONSTANT' but '< Node[] >' "
+            "does not have 'READ' access." in str(err.value))


picky but, since we're having to use debug_string() here, perhaps s/have/contain a/

arporter · 2025-12-15T16:48:19Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+    _type_of_named_arg_with_optional_kind_and_dim,
+    _type_with_specified_precision_and_optional_dim,
+    _type_of_scalar_with_optional_kind,
    _get_intrinsic_of_argname_kind_with_optional_dim,


Can you think of good names for the two "_get_intrinsic_xxxx" methods to match the _type_of... scheme that we now have?

arporter · 2025-12-15T16:49:10Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

    call = psyir.walk(IntrinsicCall)[0]
    assert isinstance(call.datatype, UnresolvedType)

+    # ValueError test.


Please extend the comment to say that you deliberately don't use the create method so as to make a malformed IntrinsicCall.

arporter · 2025-12-15T16:49:54Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+            "likely due to not fully initialising the intrinsic correctly."
+            in str(err.value))
+
+    # Test that when we get a ValueError due to unresolved/unsupported types


s/a ValueError/an AttributeError/ I think?

arporter · 2025-12-15T17:00:14Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+    code = """subroutine test
+    integer :: i
+
+    i = REAL(CMPLX(1.0,1.0))


You're not going to like this but...in situations like this, we do actually know part of the type of the result, even if we don't know the type of the input. e.g. here, we know we will have a real scalar of unknown precision. While we're deep into this, it might be worth having a look to see how much work it would be to do better. I'm also unsure whether 'doing better' would actually make any practical difference i.e. whether there are any situations where having some type information would be better than having none?

I've changed my mind - I think this is already big enough and so we can postpone investigation of this until such time as we have a case where we know we could do better.

arporter · 2025-12-15T19:08:41Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+    integer :: c
+    c = DOT_PRODUCT(a,b)
+    end subroutine x""",
+            # DOT_PRODUCT RETURN TYPE is Scalar type of input 1.


Unfortunately life is complicated:

we do already have the technology for determining the type of scalar a * scalar b so we can probably generalise that/factor it out to make it available here.

arporter · 2025-12-15T19:09:50Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+    assert res.shape[0] == ArrayType.Extent.DEFERRED
+
+
+# FIXME Do we need ANINT (also REAL) tests (Reviewer/codecov decision).


If they follow the same rules and use the same utilities as other methods then I think you can skip them.

arporter · 2025-12-15T19:20:33Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+                and res.shape[0].lower.value == "1"
+            ),
+        ),
+        # TODO #2823 Can't do this test yet.


Can we do it now?

arporter · 2025-12-15T19:22:11Z

src/psyclone/tests/psyir/nodes/intrinsic_call_test.py

+    fortran_reader, code, intrinsic, expected
+):
+    """Test the specific return types of IntrisicCalls that aren't recognised
+    correctly by fparser."""


Do you mean the fparser2 frontend here?

arporter · 2025-12-15T19:24:03Z

src/psyclone/tests/psyir/transformations/intrinsics/sign2code_trans_test.py

-      ! Can't handle because we don't know the type of MAX or ABS
-      ztmp1 = SIGN( MAX(ABS(ztmp1),1.E-6_wp), ztmp1 )
+      ! Can't handle because we don't know the type of thing
+      ztmp1 = SIGN( thing, ztmp1 )


Just out of interest, what happens if we do have the original argument (MAX(ABS(ztmp1)))? Might be worth having a separate test for that.

LonelyCat124 force-pushed the 3060_intrinsic_types branch from 5911f97 to 9f0799f Compare September 2, 2025 15:43

arporter reviewed Sep 3, 2025

View reviewed changes

src/psyclone/psyir/nodes/intrinsic_call.py Outdated Show resolved Hide resolved

arporter reviewed Sep 3, 2025

View reviewed changes

This was referenced Sep 4, 2025

Function Calls vs ArrayAccessors vs Type Constructors #2823

Closed

(Closes #3087) Handle expressions in the kind attribute of a declaration #3110

Merged

LonelyCat124 marked this pull request as ready for review September 8, 2025 09:02

LonelyCat124 added the ready for review label Sep 8, 2025

LonelyCat124 requested a review from sergisiso September 8, 2025 09:02

LonelyCat124 mentioned this pull request Sep 8, 2025

(Closes #3060) reference accesses for intrinsicCall #3125

Draft

LonelyCat124 changed the title ~~(Closes #3060) intrinsic return types and reference accesses~~ (Towards #3060) intrinsic return types Sep 8, 2025

arporter added under review and removed ready for review labels Sep 10, 2025

arporter requested changes Sep 11, 2025

View reviewed changes

arporter removed the under review label Sep 11, 2025

Remaining coverage

d5f85b1

LonelyCat124 added ready for review and removed reviewed with actions labels Dec 2, 2025

LonelyCat124 requested a review from arporter December 2, 2025 16:14

arporter added under review and removed ready for review labels Dec 8, 2025

arporter requested changes Dec 10, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Dec 10, 2025

LonelyCat124 added 2 commits December 11, 2025 13:01

Changes towards review

a26d0e0

Merge branch 'master' into 3060_intrinsic_types

ef04578

LonelyCat124 added 4 commits December 11, 2025 16:40

Merge branch 'master' into 3060_intrinsic_types

91aa922

Changes towards review

5ad8445

linting mistake

9f4270e

Merge branch '3060_intrinsic_types' of github.com:stfc/PSyclone into …

eb4021b

…3060_intrinsic_types

LonelyCat124 requested a review from arporter December 12, 2025 14:00

LonelyCat124 added ready for review and removed reviewed with actions labels Dec 12, 2025

LonelyCat124 added 2 commits December 12, 2025 14:21

Missing coverage and fixing unneeded errors

a0bc0d6

linting

242158d

Missing unit test

98cc594

arporter added under review and removed ready for review labels Dec 15, 2025

arporter requested changes Dec 15, 2025

View reviewed changes

Merge branch 'master' into 3060_intrinsic_types

4f76d51

arporter added reviewed with actions and removed under review labels Dec 16, 2025

		return ArrayType(dtype, new_shape)


		def _get_first_argument_logical_kind_with_optional_dim(node) -> DataType:

		assert res.shape[0] == ArrayType.Extent.DEFERRED


		# FIXME Do we need ANINT (also REAL) tests (Reviewer/codecov decision).

(Towards #3060) intrinsic return types #3119

Are you sure you want to change the base?

(Towards #3060) intrinsic return types #3119

Conversation

LonelyCat124 commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LonelyCat124 commented Sep 2, 2025

Uh oh!

LonelyCat124 commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LonelyCat124 commented Sep 2, 2025

Uh oh!

LonelyCat124 commented Sep 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arporter commented Sep 3, 2025

Uh oh!

arporter commented Sep 3, 2025

Uh oh!

arporter commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LonelyCat124 commented Sep 3, 2025

Uh oh!

LonelyCat124 commented Sep 3, 2025

Uh oh!

codecov bot commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented Sep 3, 2025

Uh oh!

LonelyCat124 commented Sep 4, 2025

Uh oh!

LonelyCat124 commented Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LonelyCat124 commented Sep 9, 2025

Uh oh!

LonelyCat124 commented Sep 11, 2025

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

LonelyCat124 commented Sep 2, 2025 •

edited

Loading

LonelyCat124 commented Sep 2, 2025 •

edited

Loading

arporter commented Sep 3, 2025 •

edited

Loading

codecov bot commented Sep 3, 2025 •

edited

Loading

LonelyCat124 commented Sep 8, 2025 •

edited

Loading

LonelyCat124 commented Dec 11, 2025 •

edited

Loading