(Towards #2577) Add `pointer` and `target` attributes in datatypes, frontend and backend by JulienRemy · Pull Request #2822 · stfc/PSyclone

JulienRemy · 2024-12-09T16:47:51Z

This starts implementing point 2 of Sergi's list in #2577 by adding new attributes to datatypes, as well as frontend and backend support for them.

Note that is_potentially_aliased is not used anywhere, except to fix an existing Matmul2CodeTrans test.
So this does not check for aliasing anywhere else for now. Stated differently, this might be dangerous to merge as it effectively supports pointers in PSyIR but does not yet restrict transformations to non-aliased variables.

…ureType.ComponentType`, frontend and backend. NOTE: tests not done.

…nstead of Symbols. Some tests.

codecov · 2024-12-09T17:36:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.88%. Comparing base (1e6b095) to head (32d451b).
Report is 184 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2822      +/-   ##
==========================================
- Coverage   99.88%   99.88%   -0.01%     
==========================================
  Files         357      357              
  Lines       49742    49960     +218     
==========================================
+ Hits        49686    49903     +217     
- Misses         56       57       +1

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

arporter

Very nicely done, thanks Julien. Implementation is good, tests good (just need a bit more coverage). As you say in a previous comment, I am concerned about putting this onto master as is as it opens the door to potential problems. Therefore, I think we do have to tackle making sure that transformations that care about this do the right thing (at this stage, that can simply be refuse to do anything). I suggest going through the list in the Reference Guide:

and checking whether or not they should care about aliasing.

arporter · 2025-01-21T09:28:17Z

src/psyclone/psyir/frontend/fparser2.py

+                    has_pointer_attr = True
+                elif normalized_string == "target":
+                    has_target_attr = True
+                elif isinstance(attr, (Fortran2003.Attr_Spec,


Since you do normalized_string = str(attr).lower().replace(' ', '') in both cases, can you now remove the elif isinstance(attr, (Fortran2003.xxxxx)) and hoist the if normalized_string == "save": up a level?

arporter · 2025-01-21T09:35:49Z

src/psyclone/psyir/frontend/fparser2.py

                    f"SAVE and PARAMETER attributes are not compatible but "
                    f"found:\n {decl}")

+            # TODO check these are needed


I think they are. I bet fparser doesn't check. Please update the comment.

arporter · 2025-01-21T09:36:31Z

src/psyclone/psyir/frontend/fparser2.py

+                    f"found:\n {decl}")
+            if has_pointer_attr and has_constant_value:
+                raise GenerationError(
+                    f"POINTER and PARAMETER attributes are not compatible but "


Probably also true for TARGET and PARAMETER?

arporter · 2025-01-21T09:42:51Z

src/psyclone/psyir/frontend/fparser2.py


        # 2: Remove any unsupported attributes
-        unsupported_attribute_names = ["pointer", "target", "optional"]
+        unsupported_attribute_names = ["asynchronous", "volatile", "optional"]


Possibly "contiguous" too although @hiker wants to add support for that soon I think.

arporter · 2025-01-21T09:44:15Z

src/psyclone/psyir/symbols/data_type_symbol.py

+    :param is_target: whether this is a target symbol.
+    :type is_target: bool

    '''


Please document the new :raises: that you've added.

arporter · 2025-01-21T10:14:42Z

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

-    # Test that when it is part of an UnsupportedType (target attribute in
-    # this case) it becomes an UnknownInterface.
-    reader = FortranStringReader("integer, target :: var5")
+    # Test that when it is part of an UnsupportedType (asynchronous attribute


s/asynchronous/volatile/

arporter · 2025-01-21T10:16:19Z

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

-            "INTEGER, PRIVATE, DIMENSION(3), POINTER :: g")
+            "INTEGER, PRIVATE, DIMENSION(3), ASYNCHRONOUS :: g")

    # Test with unsupported intrinsic type. Note the space before complex


Please add/extend tests that the partial datatype of an UnsupportedFortranType correctly captures the new is_pointer/is_target properties of a declaration.

arporter · 2025-01-21T10:18:05Z

src/psyclone/tests/psyir/symbols/data_type_symbol_test.py

+    with pytest.raises(ValueError) as err:
+        _ = DataTypeSymbol("my_type", UnresolvedType(), is_pointer=True,
+                           is_target=True)
+    assert ("A symbol cannot be both a pointer and a target" in str(err.value))


Useful to include the name of the proposed symbol in this error message.

arporter · 2025-01-21T10:20:20Z

src/psyclone/tests/psyir/symbols/datatype_test.py

    assert ("__str__" in msg)


+def test_datatype_is_pointer_is_target():


As commented on in the source, an unresolved type is just that - if we have any information on it then that goes in its partial_datatype (although I think only an UnsupportedFortranType has that).

arporter · 2025-01-21T11:26:00Z

src/psyclone/psyir/transformations/intrinsics/matmul2code_trans.py

+                f"Expected result and operands of MATMUL IntrinsicCall to "
+                f"be references to arrays that are not potentially aliased "
+                f"but found '{result.symbol}', '{matrix1.symbol}' and "
+                f"'{matrix2.symbol}'.")


probably worth adding "...symbol}' in '{node.debug_string()}'." to give more information.

sergisiso

I see @arporter already reviewed it, so I will let him continue being the reviewer for this, but I had some comments already stored in github so I will push them anyway (even though some are the same as @arporter ). The reason I never finished this review is that it was unclear to me if the "is a pointer" attribute had to be part of the symbol itself or the datatype of the symbol. And I think it would be good to discuss this before we commit to a solution.

sergisiso · 2024-12-18T10:17:48Z

src/psyclone/psyir/frontend/fparser2.py

+                # NOTE: for a routine declaration, 'POINTER' or 'TARGET' is
+                # an Attr_Spec, but in a derived type declaration component
+                # it is not.
+                normalized_string = str(attr).lower().replace(' ', '')
+                if normalized_string == "pointer":
+                    has_pointer_attr = True
+                elif normalized_string == "target":
+                    has_target_attr = True


Can this not be part of the normalized_string checks inside the:
if isinstance(attr, (Fortran2003.Attr_Spec, Fortran2003.Component_Attr_Spec)

Looking at the standard for derived type it should be in "Component_Attr_Spec" so I don't know why this is different than "save", "parameter", ...

sergisiso · 2024-12-18T10:24:07Z

src/psyclone/psyir/frontend/fparser2.py

                    f"SAVE and PARAMETER attributes are not compatible but "
                    f"found:\n {decl}")

+            # TODO check these are needed


I tried with 'gfortran' and they are both errors, but fparser does not report them, so it is good to have them here. You can remove the TODO.

Target and parameter together also fails, can you add it here?

sergisiso · 2024-12-18T10:33:47Z

src/psyclone/psyir/transformations/intrinsics/matmul2code_trans.py

+        :raises TransformationError: if any of the arrays are potentially \
+            aliased.
+        :raises TransformationError: if sub-sections of an array are present \


You don't need the backslashes

sergisiso · 2024-12-18T11:01:26Z

src/psyclone/psyir/symbols/datasymbol.py

+
+    @property
+    def is_unresolved(self):
+        '''
+        :returns: whether the Symbol has an UnresolvedInterface or its
+                  datatype is an UnresolvedType.
+        :rtype: bool
+        '''
+        # Import here to avoid circular dependencies
+        # pylint: disable=import-outside-toplevel
+        from psyclone.psyir.symbols.datatypes import UnresolvedType
+        return (super().is_unresolved
+                or isinstance(self.datatype, UnresolvedType))


I would prefer to remove this, as it changes the definition of the super class which is that this only checks for UnresolvedInterface.

I suppose you needed this in some place that then checks for properties of the datatype (e.g. symbol.datatype. ...), I think you will need to do this check there. Would that work?

JulienRemy and others added 9 commits July 8, 2024 15:12

#2577 Add support for pointer and target in DataSymbol, `Struct…

b97367b

…ureType.ComponentType`, frontend and backend. NOTE: tests not done.

#2577 Reimplement pointer and target attributes in DataType(Symbol) i…

774274a

…nstead of Symbols. Some tests.

#2577 Declaration of pointer with initial value.

c89085e

#2577 Edit gocean tests w.r.t. new supported attributes.

45c4b35

Merge branch 'master' into 2577_pointer_target_attrs

8503a7d

# 2577 Fix ArrayType copy.

1b6a426

Merge branch 'master' into 2577_pointer_target_attrs

95b6f73

#2577 Fix structures and one test. Breaks get_callee for now.

d9f0a67

#2577 Move .is_unresolved w.r.t. Interface AND DataType to DataSymbol.

8052b64

JulienRemy added the ready for review label Dec 9, 2024

JulienRemy requested review from arporter and sergisiso December 9, 2024 16:47

JulienRemy and others added 2 commits December 9, 2024 17:48

Merge branch 'master' into 2577_pointer_target_attrs

826bbc1

#2577 Flake8 formatting

7c43dcf

JulienRemy removed the ready for review label Dec 9, 2024

#2577 More test coverage

32d451b

JulienRemy added the ready for review label Dec 10, 2024

sergisiso added under review and removed ready for review labels Dec 16, 2024

sergisiso added ready for review and removed under review labels Jan 10, 2025

arporter added under review and removed ready for review labels Jan 21, 2025

arporter requested changes Jan 21, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Jan 21, 2025

sergisiso requested changes Jan 21, 2025

View reviewed changes

		assert ("__str__" in msg)


		def test_datatype_is_pointer_is_target():

Conversation

JulienRemy commented Dec 9, 2024

Uh oh!

codecov bot commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 9, 2024 •

edited

Loading