Replace ref by literal #2828

hbrunie · 2024-12-17T10:39:29Z

Replacing PsyIR Reference by PsyIR Literal when it is easy to do so.
Same file, same subroutine, or subroutine contained in module.
Build a param_table with symbol name and Literal, then go through psyir references and symbol_table array type shapes to look for ref with name contained in param_table.

Issues:

it seems that initial value will always be a literal, is this expected behavior? If so I can remove a test raise error not triggered yet.
I might have found a bug: `character(len=4), parameter :: x = "toto" is not parsed a a is_constant.

arporter · 2024-12-17T10:43:02Z

it seems that initial value will always be a literal, is this expected behavior? If so I can remove a test raise error not triggered yet.

Thanks Hugo. I'm pretty sure we allow any PSyIR expression as an initial value?

* Thanks Andrew, indeed, BinaryOp can be an valid initial_value

hbrunie · 2024-12-17T10:53:53Z

it seems that initial value will always be a literal, is this expected behavior? If so I can remove a test raise error not triggered yet.

Thanks Hugo. I'm pretty sure we allow any PSyIR expression as an initial value?

Thanks. I made a test showing the current limit of this MR with respect to non-literal initial value (for example a binary operation).

But I don't understand why
character (len=4), parameter :: x ="toto" does not make a constant symbol with initial_value being

arporter · 2024-12-17T11:10:42Z

But I don't understand why
character (len=4), parameter :: x ="toto" does not make a constant symbol with initial_value being

I think that's because we don't support the len=xxx specification on a character declaration.

codecov · 2024-12-17T11:52:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.89%. Comparing base (6afc06a) to head (bd12495).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2828   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files         359      360    +1     
  Lines       51073    51143   +70     
=======================================
+ Hits        51021    51091   +70     
  Misses         52       52

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

hbrunie · 2024-12-17T17:39:06Z

But I don't understand why
character (len=4), parameter :: x ="toto" does not make a constant symbol with initial_value being

I think that's because we don't support the len=xxx specification on a character declaration.

That's ok. I don't deal with "complex" right hand side for now. If this is not a Literal, the parameter is just ignored.

arporter

Good stuff Hugo, thanks for this.
Mostly my comments are about docstrings :-)
As you hint in one of your comments, I'm wondering whether we should always consider all constant symbols that are in scope, rather than explicitly looking at the Routine and Container symbol tables?
I also think it would be better if we quietly skipped constant symbols with a non-Literal value rather than raise an error. That would enable the transformation to do as much as it can rather than aborting early.
Coverage is good and the new test file fully covers the new source file.
If you feel brave, it would be good to try the transformation 'for real' by adding it to the transformation script that we use for NEMO in the integration tests. You could add it immediately after the call to enhance_tree_information:

PSyclone/examples/nemo/scripts/omp_gpu_trans.py

Lines 85 to 94 in 9b8a0ba

    
           enhance_tree_information(subroutine) 
        
           normalise_loops( 
        
                   subroutine, 
        
                   hoist_local_arrays=True, 
        
                   convert_array_notation=True, 
        
                   loopify_array_intrinsics=True, 
        
                   convert_range_loops=True, 
        
                   hoist_expressions=True 
        
           )

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

hbrunie · 2025-01-20T10:07:26Z

Good stuff Hugo, thanks for this. Mostly my comments are about docstrings :-) As you hint in one of your comments, I'm wondering whether we should always consider all constant symbols that are in scope, rather than explicitly looking at the Routine and Container symbol tables? I also think it would be better if we quietly skipped constant symbols with a non-Literal value rather than raise an error. That would enable the transformation to do as much as it can rather than aborting early. Coverage is good and the new test file fully covers the new source file. If you feel brave, it would be good to try the transformation 'for real' by adding it to the transformation script that we use for NEMO in the integration tests. You could add it immediately after the call to enhance_tree_information:

PSyclone/examples/nemo/scripts/omp_gpu_trans.py

Lines 85 to 94 in 9b8a0ba

enhance_tree_information(subroutine)

normalise_loops(

subroutine,

hoist_local_arrays=True,

convert_array_notation=True,

loopify_array_intrinsics=True,

convert_range_loops=True,

hoist_expressions=True

)

I am not sure Andrew about what you meant by : " should we always consider all constant symbols that are in scope, rather than explicitly looking at the Routine and Container symbol tables?"
Could you explain?

Besides, when you say "quietly skipping" does it mean that if the transformation finds a non-literal symbol it should not even throw a warning?
I thought that this could be handled by the transformation caller with exception catchers.
But I will do the change, and skip it quietly.

avoid error prone string comparison

hbrunie · 2025-01-23T16:25:24Z

@arporter I think I addressed all your concerns. Please let me know. Best, Hugo.

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

arporter · 2025-01-30T09:38:45Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

+        ## NOTE: (From Andrew) We may want to look at all symbols in scope
+        # rather than just those in the parent symbol table?
+        if node.parent is not None and isinstance(node.parent, Container):
+            if node.parent.symbol_table is not None:


A Container is always constructed with a SymbolTable so this check should be unnecessary?

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

arporter · 2025-01-30T10:28:25Z

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

+    rbbl = ReplaceReferenceByLiteralTrans()
+    rbbl.apply(routine_foo)
+    written_code = fortran_writer(routine_foo.ancestor(Container))
+    print(written_code)


Please rm the print

This one's still here?

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

arporter · 2025-01-30T10:34:24Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py


-    """
+    _ERROR_MSG = (
+        "Psyclone(ReplaceReferenceByLiteral): only "


For consistency with other transformations, please change this to "ReplaceReferenceByLiteralTrans: only..."

Almost, see my earlier comments about simplifying by using self.name.

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

arporter

Looking good, thanks Hugo. I think it's a good idea to add the warnings to the generated code. In my comment about outer scopes, I was thinking we could just create the mapping for all symbols in scope (by recursing up through parent symbol tables), while ensuring that we don't conform to scoping rules (i.e. the value associated with the innermost scope takes precedence). However, it's a small point and this PR is probably already sufficient as it is.
Just a few minor things to tidy up.
I'll fire off the integration tests now.
As a reminder - please leave it to me to 'resolve' converstations as that makes it much easier to check everything :-)

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

arporter · 2025-01-30T10:50:01Z

I've created #2879 to run the integration tests (because you can't do it for a PR from a fork).

arporter · 2025-01-30T14:35:54Z

Integration tests were green :-)

arporter · 2025-02-14T09:20:40Z

Hi @hbrunie, I can't remember: did you say this was ready for review now? (If you could set the label on the PR that really helps me see what I need to do next.)

hbrunie · 2025-02-14T09:28:34Z

Hi @hbrunie, I can't remember: did you say this was ready for review now? (If you could set the label on the PR that really helps me see what I need to do next.)

Yes sorry, I do it now :-).

arporter

Thanks Hugo. Very, very nearly there now. Just a little bit more tidying and then this is done.

arporter · 2025-02-14T09:44:29Z

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

    assert "integer, dimension(10,a) :: array" in written_code
    assert "call foo(2 + 3)" in written_code


+@pytest.mark.xfail


Thanks for adding this. We've found (from painful experience) that marking a whole test as 'xfail' often means that it can fail for a reason we don't expect and we don't notice. Therefore, please could you alter it by putting the 'xfail' inside the test. Have a look at e.g. test_apply_gocean_kern in syir/transformations/inline_trans_test.py. Also, please add a reason to the xfail (as is done in that example).

arporter · 2025-02-14T09:49:24Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

                if param_table.get(sym_name):
                    message = (
-                        "Psyclone(ReplaceReferenceByLiteralTrans):"
-                        + f" Symbol already found {sym_name}."
+                        f"{ReplaceReferenceByLiteralTrans._ERROR_MSG_START}"


You can use self.name here instead of this _ERROR_MSG_START - that will give the name of the Transformation.

arporter · 2025-02-14T09:49:39Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

                    message = (
-                        ReplaceReferenceByLiteralTrans._ERROR_MSG
-                        + f"{sym_name} is assigned "
+                        ReplaceReferenceByLiteralTrans._ERROR_MSG_ONLY_INITVAL


self.name here too.

arporter · 2025-02-14T09:51:20Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

+        :param param_table: map of parameters to Literal values.
+
+        :return: the new shape with any references to constants replaced by
+        their Literal values.


This continued line needs to be indented.

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py

arporter · 2025-02-14T09:56:31Z

src/psyclone/tests/psyir/transformations/replace_reference_by_literal_trans_test.py

+    rbbl = ReplaceReferenceByLiteralTrans()
+    rbbl.apply(routine_foo)
+    written_code = fortran_writer(routine_foo.ancestor(Container))
+    print(written_code)


This one's still here?

arporter · 2025-02-14T09:58:29Z

src/psyclone/psyir/transformations/replace_reference_by_literal_trans.py


-    """
+    _ERROR_MSG = (
+        "Psyclone(ReplaceReferenceByLiteral): only "


Almost, see my earlier comments about simplifying by using self.name.

hbrunie added 4 commits December 16, 2024 18:13

[ReplaceRefByLiteral] transformation first draft

3047fdd

[ReplaceRefByLiteral] replace in ArrayType shape.

04573fe

[gitignore] venv and .vscode

4ee2824

[ReplaceRefByLiteral] initial value always literal

8d3c9b4

hbrunie requested a review from arporter December 17, 2024 10:39

hbrunie self-assigned this Dec 17, 2024

hbrunie requested a review from sergisiso December 17, 2024 10:40

hbrunie added ready for review PSyIR Core PSyIR functionality labels Dec 17, 2024

hbrunie added 2 commits December 17, 2024 11:44

[ReplaceRefByLiteral] 80 chars exceeded

fdb3738

[ReplaceRefByLiteral] add test showing limit of trans

b19b6a7

* Thanks Andrew, indeed, BinaryOp can be an valid initial_value

[ReplaceRefByLit] flake8

bfe75cc

[RefbyLiteral] flake8 + test sucess

5b06c5b

Merge branch 'master' into replace_ref_by_literal

5387311

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

arporter requested changes Jan 8, 2025

View reviewed changes

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

hbrunie added 2 commits January 15, 2025 14:37

Merge branch 'master' into replace_ref_by_literal

dcf29a4

stfc#2828 fix license date.

ff98ba7

hbrunie added 3 commits January 20, 2025 11:09

stfc#2828 fix module description.

c31f4b9

stfc#2828 class docstring and import sorted.

257aac7

stfc#2828 fix indent

9e41bcc

stfc#2828 fix sphinx make html

207cda3

arporter mentioned this pull request Jan 21, 2025

Support optional and named arguments in inlining #2801

Open

hbrunie added 2 commits January 23, 2025 10:28

stfc#2828 fix flake8

b134774

stfc#2828 introducing cls constant for test

725b03d

avoid error prone string comparison

hbrunie removed the reviewed with actions label Jan 23, 2025

hbrunie requested a review from arporter January 23, 2025 16:24

arporter added the under review label Jan 28, 2025

arporter reviewed Jan 30, 2025

View reviewed changes

arporter requested changes Jan 30, 2025

View reviewed changes

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

arporter temporarily deployed to integration January 30, 2025 10:48 — with GitHub Actions Inactive

hbrunie and others added 5 commits February 5, 2025 17:43

stfc#2828 fix review requests

830a419

stfc#2828 flake8

340e0eb

stfc#2828 fix python3.8 dict into Dict for typing

53e6a83

Merge branch 'master' into replace_ref_by_literal

23b601b

Merge branch 'master' into replace_ref_by_literal

bd12495

hbrunie added ready for review and removed reviewed with actions labels Feb 14, 2025

arporter added under review and removed ready for review labels Feb 14, 2025

arporter requested changes Feb 14, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Feb 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ref by literal #2828

Replace ref by literal #2828

hbrunie commented Dec 17, 2024

arporter commented Dec 17, 2024

hbrunie commented Dec 17, 2024

arporter commented Dec 17, 2024

codecov bot commented Dec 17, 2024 •

edited

Loading

hbrunie commented Dec 17, 2024

arporter left a comment

hbrunie commented Jan 20, 2025

hbrunie commented Jan 23, 2025

arporter Jan 30, 2025

arporter Jan 30, 2025

arporter Feb 14, 2025

arporter Jan 30, 2025

arporter Feb 14, 2025

arporter left a comment

arporter commented Jan 30, 2025

arporter commented Jan 30, 2025

arporter commented Feb 14, 2025

hbrunie commented Feb 14, 2025

arporter left a comment

arporter Feb 14, 2025

arporter Feb 14, 2025

arporter Feb 14, 2025

arporter Feb 14, 2025

arporter Feb 14, 2025

arporter Feb 14, 2025

	enhance_tree_information(subroutine)

	normalise_loops(
	subroutine,
	hoist_local_arrays=True,
	convert_array_notation=True,
	loopify_array_intrinsics=True,
	convert_range_loops=True,
	hoist_expressions=True
	)

Replace ref by literal #2828

Are you sure you want to change the base?

Replace ref by literal #2828

Conversation

hbrunie commented Dec 17, 2024

arporter commented Dec 17, 2024

hbrunie commented Dec 17, 2024

arporter commented Dec 17, 2024

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

hbrunie commented Dec 17, 2024

arporter left a comment

Choose a reason for hiding this comment

hbrunie commented Jan 20, 2025

hbrunie commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arporter left a comment

Choose a reason for hiding this comment

arporter commented Jan 30, 2025

arporter commented Jan 30, 2025

arporter commented Feb 14, 2025

hbrunie commented Feb 14, 2025

arporter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2024 •

edited

Loading