cgen: return type and var for function(s)

[MichaelSt98]

cgen: return type and var for function(s)

[github-actions]

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/269/index.html

[reuterbal]

Thanks, in principle this looks very good. But the slight back-bending in cgen to determine return type and return variable suggests to me that we are actually leaving the IR in an inconsistent state for functions, and try to work around this now instead of fixing the original problem.

I've added a suggestion how this might be remedied. Of course it's untested and might unearth other issues, but let me know what you think.

[reuterbal]

This reimplements the logic in

loki/loki/subroutine.py

Lines 324 to 332 in 31eabbe

	def return_type(self):
	"""
	Return the return_type of this subroutine
	"""
	if not self.is_function:
	return None
	if self.result_name is not None:
	return self.symbol_attrs.get(self.result_name)
	return self.symbol_attrs.get(self.name)

but handles the special case that the function name no longer matches the name of the return variable. But that seems like an inconsistent IR state.

I would suggest to instead handle that renaming in the f2c transformation before this line:

loki/loki/transform/fortran_c_transform.py

Line 413 in 31eabbe

kernel.name = f'{kernel.name.lower()}_c'

with something like this:

if kernel.is_function and kernel.result_name is None:
    kernel.result_name = kernel.name

I think that should allow you to simply write here then

Suggested change

	return_var = None
	if o.is_function:
	# Determine function result variable name
	if not (result_name := o.result_name):
	result_name = o.name.replace('_c', '')
	if result_name in o.variable_map:
	return_var = o.variable_map[result_name]
	return_type = c_intrinsic_type(return_var.type) if return_var is not None else 'void'
	if o.is_function:
	return_type = c_intrinsic_type(o.return_type)
	else:
	return_type = 'void'

[reuterbal]

debug leftover?

[reuterbal]

With the above change, this would then become:

Suggested change

	if return_var is not None:
	body += [self.format_line(f'return {return_var.name.lower()};')]
	if o.result_name is not None:
	body += [self.format_line(f'return {o.result_name.lower()};')]

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.85%. Comparing base (31eabbe) to head (1bdb019).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
+ Coverage   92.82%   92.85%   +0.03%     
==========================================
  Files         102      102              
  Lines       18093    18158      +65     
==========================================
+ Hits        16795    16861      +66     
+ Misses       1298     1297       -1     

Flag	Coverage Δ
lint_rules	96.39% <ø> (ø)
loki	92.82% <100.00%> (+0.02%)	⬆️
transformations	92.20% <ø> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[MichaelSt98]

Do we want to double-check wether o.is_function?

[reuterbal]

Good point, since we can now rely on o.result_name to always have a sensible value for functions, we don't have to check for result_name but can rather check for is_function

[MichaelSt98]

Within FortranCTransformation

if kernel.is_function and kernel.result_name is None:
    kernel.result_name = kernel.name

not necessary anymore, since result_name is now set by default if is_function=True

[reuterbal]

Good point, since we can now rely on o.result_name to always have a sensible value for functions, we don't have to check for result_name but can rather check for is_function

[reuterbal]

Many thanks, this looks great now!

@mlange05: This implies a small IR change in the sense that functions will always record the name of the variable that contains the return value in result_name. That makes accessing this information much easier without the need to figure out which of the multiple variants of defining the result variable was used in the original Fortran.
For language compliance, the backend will only add the corresponding RESULT(<...>) appendix to the function statement if the name differs from the function name, thus retaining the original behaviour.
A positive side-effect of this is that renaming of functions can now happen without the need to update the result variable's name throughout the entire function body.
Let us know if you're happy with this!

[mlange05]

Yes, very happy with the IR / behaviour change (was planning on something like that for the eventual procedure overhaul). Great stuff, GTG from me, and thanks both! 🙏