(#Towards 2302) intrinsic argument names #3150

LonelyCat124 · 2025-09-22T16:00:04Z

Could be closes, not sure, up to review.

Work in progress on doing this, remaining steps I have for this PR:

Swap fparser2.py to use create when creating Intrinsics where possible (often I think it won't be). If not possible, we could canonicalise, I'll try that and see how it goes.
Fix remaining tests.

LonelyCat124 · 2025-09-24T08:03:26Z

I think we could also delete the IAttr class entirely and replace it with __new__ on the Intrinsic Enum, any preferences @sergisiso

LonelyCat124 · 2025-09-24T10:13:03Z

I think we could also delete the IAttr class entirely and replace it with __new__ on the Intrinsic Enum, any preferences @sergisiso

I quickly tried this and decided its worse because you can't pass named arguments to __new__, meaning we'd be stuck again with the non-named Enum argument lists which are getting too long now to be readable.

LonelyCat124 · 2025-09-25T13:50:16Z

I've fixed the remaining problems with the test suite. I expect there will be some coverage changes I need to look at next, notably I expect there are now unreachable sections of fparser2.py that I can remove, but I will wait to see what pycov says and go from there in case there are other sections I either miss testing or that can now be removed.

One option I leave up to the review is whether to add something to the backend to not output argument names, as we will now be very explicit and add argument names wherever possible (assuming an IntrinsicCall has been canonicalised).

codecov · 2025-09-25T14:02:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.90%. Comparing base (5ee8752) to head (be08a53).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3150    +/-   ##
========================================
  Coverage   99.90%   99.90%            
========================================
  Files         374      374            
  Lines       53011    53164   +153     
========================================
+ Hits        52958    53111   +153     
  Misses         53       53

☔ 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-26T13:39:02Z

@arporter @sergisiso This is ready for a first look now.

LonelyCat124 · 2025-11-18T14:32:55Z

I broke all the sympy stuff again when switching the default behaviour :'(

…es in its definition

LonelyCat124 · 2025-11-20T14:46:50Z

Hopefully integration works now, there was one badly defined intrinsic (capitalized argument names which docs say we shouldn't have) that I've fixed and I'm hoping was the remaining issue.

LonelyCat124 · 2025-11-20T16:01:11Z

@arporter I think this is ready for another look now - I've fixed the issues discussed and the NEMO4 integration tests work - I'm waiting on the others to finish running but the first LFRic attempt had the signal 11 issue from the Nvidia compiler so I've set it rerunning.

arporter

Thanks very much Aidan. Pretty much there now, just needs some tidying and perhaps a little refactor in the FortranWriter/SympyWriter handlers for IntrinsicCall.

arporter · 2025-11-24T11:48:58Z

doc/developer_guide/psyir.rst

-fails, then PSyclone will not create an ``IntrinsicCall`` corresponding to
-the input. This canonicalisation is required to guarantee correct behaviour
-when computing reference_accesses or the return type of an Intrinsic.
+will convert all of the arguments to be named arguments. If canonicalisation


Please could you reword this whole section (L597-) as I think we've agreed that we're not calling it "canonicalisation" any more?

arporter · 2025-11-24T12:10:57Z

src/psyclone/configuration.py

        # By default, the PSyIR backends output argument names on (most)
        # IntrinsicCalls. This option enables control of that behaviour.
-        self._backend_intrinsic_named_kwargs = True
+        self._backend_intrinsic_named_kwargs = False


Comment above needs changing as the default is now not to put unnecessary argument names?

arporter · 2025-11-24T12:14:29Z

src/psyclone/generator.py

    # Record any intrinsic output format settings.
-    if "disable_named_intrinsic_args" in args:
+    if "backend_add_all_intrinsic_arg_names" in args:
        # The backend won't attempt to add names to required


Pls update this comment too :-)

arporter · 2025-11-24T12:15:24Z

src/psyclone/generator.py

-            Config.get().backend_indentation_disabled = True
+    # A command-line flag overrides the setting in the Config file (if
+    # any).
+    if "backend_disable_validation" in args:


Please could you group this with the bit setting backend_intrinsic_named_kwargs above so that all the 'backend' options are in one place in the code.

arporter · 2025-11-24T12:42:40Z

src/psyclone/psyir/backend/fortran.py

            # Config says to avoid outputting argument names where
            # possible.
            try:
                # Canonicalisation handles any error checking we might


s/Canonicalisation/compute_argument_names()/

arporter · 2025-11-24T14:32:17Z

src/psyclone/psyir/backend/sympy_writer.py


        :returns: the SymPy representation for the Intrinsic.

        '''


Do we still need this list of special routines (and if so, please add a comment)?

I think so yes - these are the intrinsics that require a Call XXXX format, so they need to be handled differently. I think the sympy stuff doesn't work for them, but we should still be precise I think.

(I could be wrong here maybe, I don't really understand the goal of the sympy output for intrinsics that sympy doesn't recognize)

arporter · 2025-11-24T14:37:37Z

src/psyclone/psyir/symbols/intrinsic_symbol.py

+
+        :returns: A symbol object with the same properties as this
+                  symbol object.
+        :rtype: :py:class:`psyclone.psyir.symbols.IntrinsicSymbol`


Typehint instead pls (may need a from __future__ import annotations)

arporter · 2025-11-24T14:48:40Z

src/psyclone/tests/psyir/backend/sympy_writer_test.py

+
+def test_sym_writer_intrinsiccall_node(fortran_reader):
+    '''Handle unlikely edge cases for intrinsiccall node.'''
+    code = """subroutine test


Please could you specify what the 'unlikely edge cases' are.

arporter · 2025-11-24T14:56:27Z

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

+# Author A. B. G. Chalk, STFC Daresbury Lab
+# -----------------------------------------------------------------------------
+
+''' Perform py.test tests on the psygen.psyir.symbols.intrinsicsymbol file '''


This should say "....intrinsic_symbol file" and this file should be named "intrinsic_symbol_test.py".

arporter · 2025-11-24T14:58:12Z

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

+    intrinsic = psyir.walk(IntrinsicCall)[0]
+    assert isinstance(intrinsic.routine.symbol, IntrinsicSymbol)
+    isym = intrinsic.routine.symbol
+    copy = isym.copy()


Pls could you check that we do have a copy, i.e.:

assert copy is not isym assert isym.datatype is not copy.datatype etc.

LonelyCat124 · 2025-11-24T16:10:05Z

@arporter I've addressed the comments now - the only one I'm not sure about is the sympy writer one (w.r.t what the sympy writer should output for these things it doesn't support). Let me know what you think.

LonelyCat124 added 2 commits September 22, 2025 16:56

Initial changes for IntrinsicCall and named arguments

7af3005

linting

5dd0720

LonelyCat124 self-assigned this Sep 22, 2025

LonelyCat124 added in progress adjoint labels Sep 22, 2025

LonelyCat124 added 5 commits September 23, 2025 11:43

Added canonicalisation of intrinsics into the frontend

0a371ca

More test updates

3956c10

linting

fed2ccc

All but 2 tests now pass, both have behaviour changes to fix

19a5877

linting

8f3250f

LonelyCat124 added 2 commits September 24, 2025 11:13

Merge branch 'master' into 2302_intrinsic_argument_names

808aa34

Fixed remaining issues

4533878

LonelyCat124 removed the adjoint label Sep 25, 2025

LonelyCat124 added 7 commits September 26, 2025 10:33

Coverage fix

808ad04

Removed unneccessary canonicalise_minmaxsum routine

893de29

linting

c856f7c

Merge branch 'master' into 2302_intrinsic_argument_names

976b254

Merge branch 'master' into 2302_intrinsic_argument_names

befe4fa

Removed unneeded function from fparser tests

a36b037

linting

b6879e0

LonelyCat124 marked this pull request as ready for review September 26, 2025 13:39

Merge branch 'master' into 2302_intrinsic_argument_names

ba7a2cf

LonelyCat124 added ready for review and removed in progress labels Sep 26, 2025

LonelyCat124 requested a review from arporter September 26, 2025 13:39

LonelyCat124 added 3 commits November 12, 2025 16:05

Updated canoncalisation name

e6276e6

linting

fcc9892

Doc update

a50235e

LonelyCat124 requested a review from arporter November 18, 2025 14:17

LonelyCat124 added ready for review and removed reviewed with actions labels Nov 18, 2025

LonelyCat124 added 3 commits November 20, 2025 11:02

Changed default output and fixed tests

7ef9bdb

Fixed formatting

5e6ec47

Merge branch 'master' into 2302_intrinsic_argument_names

e5ece55

LonelyCat124 temporarily deployed to integration November 20, 2025 11:11 — with GitHub Actions Inactive

LonelyCat124 added 2 commits November 20, 2025 14:11

Added intrinsicsymbol copy and fixed uncovered/unneeded code

4cfefab

linting]

b8d702c

LonelyCat124 temporarily deployed to integration November 20, 2025 14:22 — with GitHub Actions Inactive

Fixed issue with an intrinsic with capitalised intrinsic argument nam…

c6b3466

…es in its definition

LonelyCat124 temporarily deployed to integration November 20, 2025 14:46 — with GitHub Actions Inactive

Fixed CMPLX

28899ef

LonelyCat124 temporarily deployed to integration November 20, 2025 15:15 — with GitHub Actions Inactive

arporter added under review and removed ready for review labels Nov 24, 2025

arporter requested changes Nov 24, 2025

View reviewed changes

arporter added reviewed with actions and removed under review labels Nov 24, 2025

Changes for review

be08a53

LonelyCat124 added ready for review and removed reviewed with actions labels Nov 24, 2025

LonelyCat124 requested a review from arporter November 24, 2025 16:10

(#Towards 2302) intrinsic argument names #3150

Are you sure you want to change the base?

(#Towards 2302) intrinsic argument names #3150

Uh oh!

Conversation

LonelyCat124 commented Sep 22, 2025

Uh oh!

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

Uh oh!

Uh oh!

LonelyCat124 commented Sep 24, 2025

Uh oh!

LonelyCat124 commented Sep 25, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented Sep 26, 2025

Uh oh!

LonelyCat124 commented Nov 18, 2025

Uh oh!

LonelyCat124 commented Nov 20, 2025

Uh oh!

LonelyCat124 commented Nov 20, 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!

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!

LonelyCat124 commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

LonelyCat124 commented Sep 24, 2025 •

edited

Loading

codecov bot commented Sep 25, 2025 •

edited

Loading