(Towards #3325) Improving .datatype to return size intrinsics instead of DEFERRED by LonelyCat124 · Pull Request #3336 · stfc/PSyclone

LonelyCat124 · 2026-02-17T15:22:03Z

Still in progress, this so far changes the arrayreference datatype to never give shapes with ArrayType.Extent in the result if we can resolve the shape.

I want to look at binaryoperation and how it chooses what result to use next, as I think we can do better than the current resul.

…taining status quo for more unknown types

LonelyCat124 · 2026-02-18T11:54:41Z

Waiting on coverage to come back, but if its ok this is ready for a first look.

I'm not totally convinced by the choice I make in BinaryOperation, so if the reviewer has a better strategy I'm happy to change it.

StructureReference already did output sizes better than ArrayReference, so from my experiment I didn't think that needed to be updated.

sergisiso · 2026-02-18T12:09:54Z

I am happy to review this, as I have been thinking what I need to redo WHEREs and it will be something similar. (I will wait for test/coverage to be reported before starting)

codecov · 2026-02-18T12:21:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.95%. Comparing base (b703387) to head (e4fb575).

❌ Your project check has failed because you have indirect coverage changes. Learn more about Unexpected Coverage Changes and reasons for indirect coverage changes.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3336      +/-   ##
==========================================
- Coverage   99.95%   99.95%   -0.01%     
==========================================
  Files         384      384              
  Lines       54225    54232       +7     
==========================================
+ Hits        54203    54208       +5     
- Misses         22       24       +2

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

sergisiso

Looks good @LonelyCat124 (although I am not convinced about the new special case - see inline)

I see this is a towards, is the goal of that issue to make "the datatype method should not propagate the DEFERRED attribute when constructing the datatype of an expression". Where are we on this?

sergisiso · 2026-02-19T09:53:14Z

src/psyclone/psyir/nodes/array_reference.py

            else:
-                base_type = self.symbol.datatype
+                # Create a copy of the base datatype.
+                base_type = self.symbol.datatype.datatype.copy()


Not the fault of this PR but I always find the datatype.datatype very confusing and I have to dig into it to understand what it tries to do (and we have it in several places). I propose renaming the ArrayType property as "elemental_type", I think it would make it more readable everywhere we use it? @arporter any opinions?

(I would also remove the comment then as is the same as the statement)

I think we already have an issue about ArrayType's subtypes so I think the improvement should be suggested there probably @sergisiso

What is the issue number, I am not able to find it.

#1857 I was thinking it sounded related to this.

sergisiso · 2026-02-19T10:01:33Z

src/psyclone/psyir/nodes/array_reference.py

+            # Special case for UnsupportedFortranType to match previous
+            # behaviour expected by lfric tests.
+            if (orig_shape and len(orig_shape) == len(shape) and isinstance(
+                self.symbol.datatype, UnsupportedFortranType) and
+                all(self.is_full_range(idx) for
+                    idx in range(len(shape)))):
+                return self.symbol.datatype
+            if (orig_shape and len(orig_shape) == len(shape) and
+                all(self.is_full_range(idx) for idx in range(len(shape)))
+                and all(isinstance(idx, ArrayType.ArrayBounds) for idx in
+                        orig_shape)):
+                # Although this access has a shape, it is for the
+                # whole array, and the original array has no ArrayType.Extent
+                # elements, so we should return the original shape as its
+                # fully defined without using size intrinsics.


I don't agree with this special-case, at least not for the reason in the comment.

Only 1 lfric test fails and is probably due to a wrong assert, if we want to test the symbol declaration type we need to do isinstance(psyir_args[4].symbol.datatype, ....

As now ref.datatype (resulting reference datatype) and ref.symbol.datatype (declaration datatype ) have slightly different meanings.

I notice there are also differences in WHERE tests that I haven't looked at, are this the same reason?

I fixed the test for the LFRic test to use symbol.datatype.

I've not looked at the WHERE tests, I don't think I changed any of those? Or if I did I missed it.

To clarify, when I tried commenting out this special case code something in the frontend where tests failed. But maybe it was unrelated - no worries if you don't see it.

The second block is needed as otherwise you get:

do widx1 = 1, SIZE(a, dim=1), 1

instead of just

do widx1 = 1, 100, 1

Oh yeah these are the tests where you now get SIZE instead of just 100 because the second block isn't the special case the original comment is referring to - its just the basic case for a full range where we have a non-allocatable array. I'll fix the comment.

sergisiso · 2026-02-19T10:41:29Z

src/psyclone/psyir/nodes/operation.py

+        # FIXME We can do better here, pick whichever we have more information
+        # about, where information is literal > reference > intrinsiccall
+
+        # If only one of the argtypes is an ArrayType then we return that


Not yours but I am confused by the use of "argtypes" in this method, can we rename it to "operand_types"

I changed it to optype, I can expand it still if you'd prefer but I'd like it to be a bit concise as a name.

sergisiso · 2026-02-19T10:46:50Z

src/psyclone/psyir/nodes/operation.py

+            shape = (argtypes[0].shape if isinstance(argtypes[0], ArrayType)
+                     else argtypes[1].shape)


Is the else in this statement ever used? the parent if already tells us that all are ArrayType. If not the comment on top should say "... the first"

I don't think it can be any more since I added the if statement so I'll remove it.

sergisiso · 2026-02-19T10:47:26Z

src/psyclone/psyir/nodes/operation.py

-        shape = (argtypes[0].shape if isinstance(argtypes[0], ArrayType) else
-                 argtypes[1].shape)
+        # FIXME We can do better here, pick whichever we have more information
+        # about, where information is literal > reference > intrinsiccall


Remove the FIXME and this comment may be better inside the else block below

Removed the whole block, the commont in the else already describes what we're doing.

sergisiso · 2026-02-19T10:49:45Z

src/psyclone/psyir/nodes/operation.py

+                     else argtypes[1].shape)
+        else:
+            # Otherwise we try to work out the best. Find out how many
+            # or non-intrinsic DataNodes shape definitions each


Remove "or"?

Yeah, it remained from a previous implementation where it was a bit more conditional, but I decided this was sufficient.

sergisiso · 2026-02-19T10:50:44Z

src/psyclone/tests/psyir/frontend/fparser2_where_handler_test.py

      end if
    enddo'''
    out = fortran_writer(psyir)
+    print(out)


sergisiso · 2026-02-19T11:00:39Z

src/psyclone/psyir/nodes/operation.py

+            # Otherwise use the default.
+            shape = (argtypes[0].shape if isinstance(argtypes[0], ArrayType)
+                     else argtypes[1].shape)
        return ArrayType(base_type, shape=shape)


Add tests in src/psyclone/tests/psyir/nodes/operation_test.py to show/test this functionality

This has shown up a slight issue where I will often get 1 + SIZE(blah) - 1 as the .upper, which doesn't show the behaviour I want so I need to add some slight improvement to the check I think.

Fixed this to get the correct behaviour. I don't love that the upper shape is "5-1+1", but I think that's a more fundamental issue with _extent and using integer types that aren't exactly INTEGER_TYPE. This test (for whatever reason) uses INTEGER_SINGLE_TYPE (which I just copied), but then this fails at comparing to "one" in ArrayMixin._extent because the precisions are different.

@sergisiso @arporter I discussed adding a "get_python_representation" (more concise name is appreciated) to the Literal class - can I add that into this PR and use it in _extent to check start/step being one? Or would you rather I make a separate PR and come back to this later.

The new implementation is a little more verbose but doesn't care about precision any more.

sergisiso · 2026-02-19T11:07:38Z

Oh and I forgot about the Coverage indirect changes. It seems they are related to WHEREs, so I would address that comment first and see if they are still there.

LonelyCat124 · 2026-02-24T14:18:34Z

@sergisiso Ready for another look now I've updated the array_mixin stuff.

LonelyCat124 added 3 commits February 17, 2026 15:17

Update to arrayreference datatype to improve functionality while main…

75723e1

…taining status quo for more unknown types

Improvement(ish) to BinaryOperation's datatype

391a92f

Fix remaining test

bfa94d8

LonelyCat124 requested review from arporter and sergisiso February 18, 2026 11:49

LonelyCat124 added the ready for review label Feb 18, 2026

LonelyCat124 marked this pull request as ready for review February 18, 2026 11:49

Missing space from test output

87ab1dc

Merge branch 'master' into 3325_datatype_enhancement

255d8d7

sergisiso added under review and removed ready for review labels Feb 19, 2026

sergisiso requested changes Feb 19, 2026

View reviewed changes

sergisiso added reviewed with actions and removed under review labels Feb 19, 2026

LonelyCat124 added 2 commits February 20, 2026 10:11

Changes for review

37e4a80

linting

75aaa78

LonelyCat124 mentioned this pull request Feb 20, 2026

(Closes #3339) Added a python representation to the Literal class #3342

Merged

LonelyCat124 added 2 commits February 24, 2026 13:47

Merge branch 'master' into 3325_datatype_enhancement

39d7c62

Updated array_mixin to be precision independent on extent sizes

e4fb575

LonelyCat124 added ready for review and removed reviewed with actions labels Feb 24, 2026

		shape = (argtypes[0].shape if isinstance(argtypes[0], ArrayType)
		else argtypes[1].shape)

Comments

Conversation

LonelyCat124 commented Feb 17, 2026

Uh oh!

LonelyCat124 commented Feb 18, 2026

Uh oh!

sergisiso commented Feb 18, 2026

Uh oh!

codecov bot commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sergisiso 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!

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

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

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

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergisiso commented Feb 19, 2026

Uh oh!

LonelyCat124 commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Feb 18, 2026 •

edited

Loading