Updates for inlining with common variables by schreiberx · Pull Request #3389 · stfc/PSyclone

schreiberx · 2026-03-30T11:28:27Z

No description provided.

codecov · 2026-03-30T11:40:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (59b71f5) to head (8bc9306).

Additional details and impacted files

@@           Coverage Diff            @@
##           master    #3389    +/-   ##
========================================
  Coverage   99.96%   99.96%            
========================================
  Files         389      389            
  Lines       54598    54710   +112     
========================================
+ Hits        54579    54692   +113     
+ Misses         19       18     -1

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

arporter

Thanks @schreiberx, I learnt (or reminded myself) quite a lot about common blocks :-)
Unfortunately, it is currently impossible to do what you want safely as PSyclone does not capture which common a variable with a 'common block' interface is associated with. Since the names of common blocks are global and don't live in the same space as variables, I think extending PSyclone to at least capture those names and the symbols associated with them should be relatively straightforward. The code in question is:

PSyclone/src/psyclone/psyir/frontend/fparser2.py

Lines 2771 to 2798 in 8dd90f5

    
           if isinstance(node, Fortran2003.Common_Stmt): 
        
               # Place the declaration statement into a UnsupportedFortranType 
        
               # (for now we just want to reproduce it). The name of the 
        
               # commonblock is not in the same namespace as the variable 
        
               # symbols names (and there may be multiple of them in a 
        
               # single statement). So we use an internal symbol name. 
        
               psyir_parent.symbol_table.new_symbol( 
        
                   root_name="_PSYCLONE_INTERNAL_COMMONBLOCK", 
        
                   symbol_type=DataSymbol, 
        
                   datatype=UnsupportedFortranType(str(node))) 
        
               # Get the names of the symbols accessed with the commonblock, 
        
               # they are already defined in the symbol table but they must 
        
               # now have a common-block interface. 
        
               try: 
        
                   # Loop over every COMMON block defined in this Common_Stmt 
        
                   for cb_object in node.children[0]: 
        
                       for symbol_name in cb_object[1].items: 
        
                           sym = psyir_parent.symbol_table.lookup( 
        
                                       str(symbol_name)) 
        
                           if sym.initial_value: 
        
                               # This is C506 of the F2008 standard. 
        
                               raise NotImplementedError( 
        
                                   f"Symbol '{sym.name}' has an initial value" 
        
                                   f" ({sym.initial_value.debug_string()}) " 
        
                                   f"but appears in a common block. This is " 
        
                                   f"not valid Fortran.") 
        
                           sym.interface = CommonBlockInterface()

Given that we've learnt to our cost that safety needs to come first, I suggest we need to extend PSyclone's handling of common a little before we can safely handle common blocks when merging tables.

arporter · 2026-03-31T15:01:14Z

+        if (isinstance(old_sym.datatype, UnsupportedFortranType) and
+                isinstance(self_sym.datatype, UnsupportedFortranType)):
+            if old_sym.datatype.declaration == self_sym.datatype.declaration:
+                # Identical COMMON-block markers – already present in self.


This comment isn't right - they are just the same unsupported type, not necessarily a common block.

In fact, this is quite confusing. I think what we have here is that the two symbols that have a name collision both have a 'common block' interface and also happen to be of UnsupportedFortranType. I think (but could be wrong) that it would also be possible for them to be of a supported type. Unfortunately, we have no way of knowing whether they are both the (same) variable from the same common block.

I'm afraid I'm still confused here. The check at L1038 means that only one or none of the clashing symbols has a common-block interface. Therefore, old_sym and self_sym can't both be common-block markers and in fact both might not be? Am I misunderstanding something?

arporter · 2026-03-31T15:25:19Z

+            for s in self.symbols if s.is_commonblock)
+
+        # Normalised names of commonblock variables in other_table.  Any
+        # overlap with self_cb_names means the COMMON block is already


Worth extending this comment to say that the same variable cannot appear in more than one common block so if they have the same name and a 'common block' interface then they must originate from the same common. I'm slightly uneasy as the actual names of symbols within a common block don't have to match: just their order and type:

So, two symbols from different scopes but with the same name and a common block interface are not guaranteed to be referring to the same thing.

Consider:

integer :: var1, var2 common /block1/ var1, var2

and (in a different scope):

real :: var1, var2 common /block2/ var1, var2

as written, the code would assume that it is safe to ignore this clash because both var1 and var2 have a 'common block' interface. Unfortunately we currently make no attempt to capture the name of a common block - the whole common /.... statement is just reproduced blindly.

schreiberx · 2026-04-01T09:06:43Z

Since this is public, please also let me add that I informed you in advance that this PR is AI-generated and not proofread.
I forgot to mention this, and I think it's important to note it here.

arporter · 2026-04-01T09:08:19Z

Since this is public, please also let me add that I informed you in advance that this PR is AI-generated and not proofread. I forgot to mention this, and I think it's important to note it here.

Thanks Martin, that's a good point. I would have worded some of my comments differently if I knew you had personally written the code :-)

schreiberx · 2026-04-02T13:34:56Z

This would solve #2554

schreiberx · 2026-04-02T14:10:18Z

@arporter Back to you.

schreiberx · 2026-04-03T13:59:38Z

I updated the tests.
I also saw from your other branch that you wanted to update some URLs, which I also did in this branch.

arporter · 2026-04-10T08:22:18Z

Thanks Martin, if you could fix the linting so that the test suite runs through then I'll take another look.

schreiberx · 2026-04-10T09:09:09Z

linting fixed.

arporter · 2026-04-15T14:08:50Z

Hi @schreiberx, please could you fix the test failure and then make sure the coverage is OK too?

… martin_common

schreiberx · 2026-04-25T21:14:24Z

Hi Andy, I fixed this and added some new code to address another issue with Croco.

arporter

Nice, thanks Martin. It's impressive to see what can be done now.
There's a bit more code moving to do and I'm still a bit worried that we'll let through overlapping common-block declarations that use different local variable names.
Apart from that, it would be great to add compilation checks to all of the new tests you've added. You need to:

Add the tmp_path test fixture;
output = fortran_writer(psyir)
Add a Compile(tmp_path).string_compiles(output)

arporter · 2026-04-28T08:22:56Z


        self_sym = self.lookup(old_sym.name)
        if old_sym.is_unresolved and self_sym.is_unresolved:
+            # Update after fixing issue #3392


Since this routine assumes check_for_clashes has been called successfully, this comment doesn't belong here. However, we could do with a comment at 1039 to say that check_for_clashes must have determined that the two variables are from the same common block.

arporter · 2026-04-28T08:31:06Z

+        if (isinstance(old_sym.datatype, UnsupportedFortranType) and
+                isinstance(self_sym.datatype, UnsupportedFortranType)):
+            if old_sym.datatype.declaration == self_sym.datatype.declaration:
+                # Identical COMMON-block markers – already present in self.


I'm afraid I'm still confused here. The check at L1038 means that only one or none of the clashing symbols has a common-block interface. Therefore, old_sym and self_sym can't both be common-block markers and in fact both might not be? Am I misunderstanding something?

arporter · 2026-04-28T08:36:09Z

                # We've dealt with Container symbols in _add_container_symbols.
                continue

+            # Avoid duplicate COMMON-block marker symbols when multiple


This new logic belongs in _handle_symbol_clash - that's where we determine whether or not to take any action to resolve a clash.

arporter · 2026-04-28T08:40:35Z

+        # (PARAMETER) symbols and redirect their references *before* the
+        # routine body is extracted, so that the extracted statements already
+        # carry references to the call-site symbols.
+        extra_skip: List[DataSymbol] = []


You can use "list" rather than "List".

arporter · 2026-04-28T08:42:23Z

+        extra_skip: List[DataSymbol] = []
+        if not parameter_cloning:
+            extra_skip = self._redirect_duplicate_parameters(
+                table, routine, routine_table)


Please don't pass routine_table - it must always be the one that belongs to routine.

arporter · 2026-04-28T09:30:56Z

+    assert result.count("parameter :: base_val") == 1
+
+
+def test_apply_parameter_cloning_false_unary_op_identical(fortran_reader,


I don't think you need this test - it's really just testing the __eq__ of DataNode.

arporter · 2026-04-28T09:31:37Z

+    # negflag depends on flag which differs, so foo's negflag must also
+    # appear (renamed), and the inlined if must use foo's (renamed) negflag.
+    assert "negflag_1" in result
+    assert "if (negflag_1)" in result


Compilation check pls.

arporter · 2026-04-28T09:32:12Z

+  constval = 7
+  b = constval_1
+
+end subroutine bar""" in result


Pls check compilation too.

arporter · 2026-04-28T09:32:26Z

+
+  b = INT(constval_1)
+
+end subroutine bar""" in result


Compilation pls.

arporter · 2026-04-28T09:34:08Z

+  end subroutine bar
+  subroutine foo(a)
+    logical, parameter :: flag = .false.
+    logical, parameter :: negflag = .not. flag


Worth doing a similar test where a kind parameter is duplicated but different between the call site and the callee, e.g.:

integer, parameter :: wp = kind(1.0d0) real(kind=wp), parameter :: pi = 3.14592

and the same in the callee but with:

integer, parameter :: wp = kind(1.0)

schreiberx · 2026-04-29T14:39:00Z

Thanks for your feedback. I'm trying to summarize all changes briefly:

The compilation feature was added to most of the inlining test cases. This requires running with pytest --compile.
I moved almost all common-related parts in the symbol table into a dedicated function. We need to treat "common" variables partly differently and moving this handling into a separate function was IMHO the best solution to keep the code clearer.
I added also further support / test cases to handle merges if the variable names aren't matching. They don't need to match; they just need to match the type.

schreiberx · 2026-04-29T16:08:29Z

@arporter Next round :-)

Updates for inlining with common variables.

b417624

arporter reviewed Mar 31, 2026

View reviewed changes

Addressing rev. comments

0ea7758

schreiberx added the ready for review label Apr 2, 2026

more updates for other cases + added test cases

2961d29

added duplicate parameter fix

7a17765

fixed issue pointed out by flake8

c6fc922

arporter added 2 commits April 10, 2026 11:12

Merge branch 'master' into martin_common

73c8e95

Merge branch 'master' into martin_common

40778f2

arporter added reviewed with actions and removed ready for review labels Apr 15, 2026

SCHREIBER Martin and others added 10 commits April 21, 2026 11:03

Merge branch 'master' into martin_common

5fb11a8

Merge branch 'master' into martin_common_parameters

f6ed6c0

minor updates & fixes

fde246d

Merge branch 'martin_common' into martin_common_parameters

2d47e40

fixed flake8 issues

d387d0e

Merge branch 'martin_common' of https://github.com/stfc/PSyclone into…

f45714d

… martin_common

added test cases for coverage

2906609

final fix

3d3af9d

coverage tests

e747e28

Merge branch 'master' into martin_common

4ddfcf1

schreiberx added ready for review and removed reviewed with actions labels Apr 21, 2026

arporter added under review and removed ready for review labels Apr 27, 2026

Merge branch 'master' into martin_common

e7c0906

arporter mentioned this pull request Apr 28, 2026

Extend CommonBlockInterface so that it holds the name of the common block #3392

Open

arporter requested changes Apr 28, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Apr 28, 2026

SCHREIBER Martin added 2 commits April 29, 2026 11:25

intermediate commit

ea1e2d2

cleanups

94c7422

SCHREIBER Martin and others added 2 commits April 29, 2026 16:41

Merge branch 'master' into martin_common

6c4fa5f

yet another test

8bc9306

schreiberx added ready for review and removed reviewed with actions labels Apr 29, 2026

	if isinstance(node, Fortran2003.Common_Stmt):
	# Place the declaration statement into a UnsupportedFortranType
	# (for now we just want to reproduce it). The name of the
	# commonblock is not in the same namespace as the variable
	# symbols names (and there may be multiple of them in a
	# single statement). So we use an internal symbol name.
	psyir_parent.symbol_table.new_symbol(
	root_name="_PSYCLONE_INTERNAL_COMMONBLOCK",
	symbol_type=DataSymbol,
	datatype=UnsupportedFortranType(str(node)))

	# Get the names of the symbols accessed with the commonblock,
	# they are already defined in the symbol table but they must
	# now have a common-block interface.
	try:
	# Loop over every COMMON block defined in this Common_Stmt
	for cb_object in node.children[0]:
	for symbol_name in cb_object[1].items:
	sym = psyir_parent.symbol_table.lookup(
	str(symbol_name))
	if sym.initial_value:
	# This is C506 of the F2008 standard.
	raise NotImplementedError(
	f"Symbol '{sym.name}' has an initial value"
	f" ({sym.initial_value.debug_string()}) "
	f"but appears in a common block. This is "
	f"not valid Fortran.")
	sym.interface = CommonBlockInterface()

		assert result.count("parameter :: base_val") == 1


		def test_apply_parameter_cloning_false_unary_op_identical(fortran_reader,

Conversation

schreiberx commented Mar 30, 2026

Uh oh!

codecov Bot commented Mar 30, 2026 • 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

schreiberx commented Apr 1, 2026

Uh oh!

arporter commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schreiberx commented Apr 2, 2026

Uh oh!

schreiberx commented Apr 2, 2026

Uh oh!

schreiberx commented Apr 3, 2026

Uh oh!

arporter commented Apr 10, 2026

Uh oh!

schreiberx commented Apr 10, 2026

Uh oh!

arporter commented Apr 15, 2026

Uh oh!

schreiberx commented Apr 25, 2026

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!

schreiberx commented Apr 29, 2026

Uh oh!

schreiberx commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Mar 30, 2026 •

edited

Loading

arporter commented Apr 1, 2026 •

edited

Loading