(#Towards 2302) intrinsic argument names

[LonelyCat124]

Could be closes, not sure, up to review.

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

1. 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.
2. Fix remaining tests.

[LonelyCat124]

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

[LonelyCat124]

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]

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]

Codecov Report

❌ Patch coverage is 99.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 99.89%. Comparing base (e4ebc9b) to head (fcc9892).
⚠️ Report is 44 commits behind head on master.

Files with missing lines	Patch %	Lines
src/psyclone/psyir/backend/fortran.py	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
- Coverage   99.90%   99.89%   -0.01%     
==========================================
  Files         376      376              
  Lines       53211    53360     +149     
==========================================
+ Hits        53159    53305     +146     
- Misses         52       55       +3     

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

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

[LonelyCat124]

@arporter I think this is ready for another look. I left a couple of things not quite as you suggested as I thought i made the help text too complicated.

This still needs integration test updates I think for NEMO - I think at the moment these will fail. @sergisiso is there an easy way for me to change the NEMO tests to use the new flag to disable intrinsic names arguments?

[sergisiso]

@sergisiso is there an easy way for me to change the NEMO tests to use the new flag to disable intrinsic names arguments?

Yes and no.

For the ones that use psyclonefc yes. You just need to add the flags to the PSYCLONE_OPTS env var.

For the ones that use the patched nemo no. But I would be in favour of moving them all to psyclonefc so that this is not a problem anymore.

[arporter]

Very nearly there. I just can't quite live with the current set of backend-related flags so am suggesting a change there - it's up for discussion. While that's rumbling in the background, we need to bite the bullet and update the integration tests so that they can be run for this PR. I think this means updating the PSyclone commands in examples/nemo/scripts/Makefile, .github/workflows/nemo_tests.yml and .github/workflows/nemo_v5_tests.yml. Unfortunately, the latter uses the native support in the NEMO buildsystem. As Sergi suggests, I think we'd be better off chaning those to use psyclonefc (see nemo_tests.yml for examples).

[arporter]

This is the only remaining sticking point. I'm not keen on having two different forms for backend options. Since we now have a nice group, we probably don't need the --backend option way of doing it. We could just have three different flags: --backend_disable_validation, --backend-disable-indentation and --backend-disable-named-intrinsic-args. This is wordy but consistent. We could shorten "backend" to "be" or "BE" perhaps? This is a change to the interface but not a big one and probably not one that affects many people. What do @LonelyCat124 @sergisiso and @hiker think?

[sergisiso]

I have no problem in moving them to independent flags grouped in the help. But I prefer "backend" rather than the abbreviations. Also "disable-named-intrinsic-args" seems like we are disabling some intrinsics, I would call it "--backend-omit-unneeded-intrinsic-arg-names" (or something shorter if the intent is clear)

[LonelyCat124]

Yeah I'm happy with those names, I'll fix those.

[LonelyCat124]

Done

[sergisiso]

and reorder arguments to match the specification from the Fortran standard.

I suppose this is a leftover?

If canonicalisation fails, then PSyclone will not ...

I think it would be more interesting to know what it will do, e.g. "PSyclone will produce and error" or "will create a CodeBlock"

[LonelyCat124]

Yes, clarified.

[sergisiso]

Yes, clarified.

I haven't seen the clarification commit yet but I have been looking at the code and it seems we don't do re-ordering anymore. The frontend (through create) just tries to match the interface and give names, and produces a CodeBlock if it can't. The transformations should be using the argument_by_name. The backend just omits names while they match the standard order, if they don't or there is any problem it continues with names. Is this a correct summary?

If that's the case, maybe we don't need the flag? This seems a good output style (no names while they match the order, named afterwards or in case of doubt). It also prevents the NEMO sign issue. And I am not too keen on having output style knobs. What do you think @arporter @LonelyCat124 about making that the backend behaviour (no options needed).

That said, NEMOv5 fails with a wrong arg-name that will need to be fixed:

errioipsl.psycloned.f90:36:34:

   36 |       ilv_max = MAX(ilv_max, None=plev)
      |                                  1
Error: Unknown argument 'none' at (1) to intrinsic max

[sergisiso]

Also, I am not sure canonicalise is the right word anymore. To me it implies reshaping which we don't do anymore, and without being familiar with this PR I probably wouldn't understand what it does without looking inside. A better name for the canonicalise method could be:
call.provide_all_argument_names()
or
call.provide_all_argument_names_or_fail() (since sometimes is used only for validation)

To be clear, I am not suggesting any implementation change, just renaming the method and associated comments.

[arporter]

It does a bit more than naming arguments: it works out which version of an intrinsic is being called. Perhaps identify... or match_interface... or something?

[LonelyCat124]

Also, I am not sure canonicalise is the right word anymore. To me it implies reshaping which we don't do anymore, and without being familiar with this PR I probably wouldn't understand what it does without looking inside. A better name for the canonicalise method could be: call.provide_all_argument_names() or call.provide_all_argument_names_or_fail() (since sometimes is used only for validation)

To be clear, I am not suggesting any implementation change, just renaming the method and associated comments.

I debated this previously when i stopped reshaping, but I felt like we still end up with a canonicalised (standard w.r.t argument names, but not argument order) version of the IntrinsicCall so I didn't rename it. I can rename if you feel strongly about it though. Either way I probably need to check over the docstring and docs.

If that's the case, maybe we don't need the flag? This seems a good output style (no names while they match the order, named afterwards or in case of doubt). It also prevents the NEMO sign issue. And I am not too keen on having output style knobs. What do you think @arporter @LonelyCat124 about making that the backend behaviour (no options needed).

It only removes required argument names (as I think optional ones should be left in generally).
I don't mind changing the default output style to that, but then I'd want to add a "leave all argument names" option (as I feel giving code output options when we have information is reasonable). I also feel like once the default behaviour is to remove argument names from required arguments if we can, then I struggle to see the same argument for not reordering and removing for consistency of output, though changing order at output-time does get a bit weird w.r.t transformations.

[sergisiso]

My problem with node.canonicalise is that is not obvious what it does or that is also a validation without reading documentation, and a more descriptive name would help. Also we have been using it in the frontend to mean "convert fparser constructs to the available psyir nodes", but here the concept is used slightly differently and it escapes the frontend, so I think a more specific name will be nicer.

as I feel giving code output options when we have information is reasonable

I will let you two decide about this. I don't think I care too much about the output style as far as is readable and reasonable. The previous backend options are to bypass issues. Going down to provide style options could make the backend much more complex and I am not sure this is the focus of psyclone. But if it is just this one it is not a big deal.

[LonelyCat124]

Yeah I'm fine to change it then. I'll look at doing that after the bug fix.

I'm also happy I think to change the default output and have an option to enable argument names separately.