This is an archive of the discontinued LLVM Phabricator instance.

[flang] Don't fold operation when shapes differ
ClosedPublic

Authored by luporl on Mar 29 2023, 1:41 PM.

Details

Summary

When folding a binary operation between two array constructors, it
is necessary to check if each value contained in the left operand
has the same rank and shape as the one on the right.
Otherwise, lowering would end up with an operation between values
of different ranks/shapes, which could result in a crash.

For instance, the following code was crashing the compiler:

integer :: x(4), y(2, 2), z(4)

z = (/x/) + (/y/)

Fixes #60229

Diff Detail

Event Timeline

luporl created this revision.Mar 29 2023, 1:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 29 2023, 1:41 PM
Herald added a subscriber: sunshaoce. · View Herald Transcript
luporl edited the summary of this revision. (Show Details)Mar 29 2023, 1:42 PM
luporl published this revision for review.Mar 29 2023, 1:45 PM
luporl added reviewers: klausler, jeanPerier.

All array constructors have rank 1 in Fortran. Items in an array constructor's list are flattened into vectors if they have rank > 1. Your example program is valid Fortran.

All array constructors have rank 1 in Fortran. Items in an array constructor's list are flattened into vectors if they have rank > 1. Your example program is valid Fortran.

@klausler, you are right, the Fortran input files are valid, the issue is that they are rewritten by expression analysis to invalid code:

Legal input:

subroutine sub(x, y)
  integer :: x(4)
  integer :: y(2,2)
  if(any((/x/)/=(/y/))) print *,'different'
end subroutine sub

Illegal rewrite that can be seen with flang-new -fc1 -fdebug-unparse on the legal input.

SUBROUTINE sub (x, y)
 INTEGER x(4_4)
 INTEGER y(2_4,2_4)
 IF (any([LOGICAL(4)::x/=y])) PRINT *, "different"
END SUBROUTINE sub

This causes x/=y, which is illegal Fortran since x and y are arrays with different ranks to hit lowering instead of (/x/)/=(/y/).
We could maybe extend lowering to support this inside an array constructor, but it will likely bring extra complexity that is seldom used, so I would advise against. Beside, I think -unparse should produce valid Fortran. So the patch here looks legitimate to me here.

Did you intend instead that the array value should have been flattened to a list [x(1), x(2), x(2), x(4)] and [y(1,1), y(2,1), y(1,2), y(2,1)] before MapOperation is called? I am not sure this is possible with the general case (i.e.: if the arrays are the result of function calls).

Did you intend instead that the array value should have been flattened to a list [x(1), x(2), x(2), x(4)] and [y(1,1), y(2,1), y(1,2), y(2,1)] before MapOperation is called? I am not sure this is possible with the general case (i.e.: if the arrays are the result of function calls).

Yes, I think that I did, when they are both constant and the operation is being folded. I may have a rewriting "optimization" in there that rewrites [x] to x for single-item array constructors, and it may need a rank check.

Okay, I took a look at that code, and I think that this fix is probably the best option without a lot of rework, so long as it compares not only ranks but also extents.

Right, I'll add the extents comparison.

I didn't realize it could be a problem, because the number of elements of the array constructors must be the same already. But code like the following is still being rewritten:

integer :: x(4), y(2)

[x, y, y] + [y, y, x]
luporl updated this revision to Diff 509788.Mar 30 2023, 12:52 PM

Add extents comparison

luporl retitled this revision from [flang] Don't fold operation when ranks differ to [flang] Don't fold operation when shapes differ.Mar 30 2023, 12:55 PM
luporl edited the summary of this revision. (Show Details)

If the unparsed output looks okay -- and maybe you should add a test for it -- then I think that you're good to go.

I added the tests marked in the comment below, to check the unparsed output. Do you think it is enough?

flang/test/Evaluate/rewrite01.f90
217–218

I added some tests here, in this subroutine. This one, specifically, checks if the operation is not rewritten when the ranks are equal but the extents differ.

klausler accepted this revision.Mar 30 2023, 1:25 PM

Thank you for debugging this problem!

This revision is now accepted and ready to land.Mar 30 2023, 1:25 PM
jeanPerier accepted this revision.Mar 31 2023, 12:04 AM

Thanks for the reviews!

This revision was automatically updated to reflect the committed changes.