Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sign2CodeTrans generates invalid code for an array argument #2768

Closed
arporter opened this issue Nov 5, 2024 · 5 comments
Closed

Sign2CodeTrans generates invalid code for an array argument #2768

arporter opened this issue Nov 5, 2024 · 5 comments
Labels
bug in progress Release Planning and creating PSyclone releases

Comments

@arporter
Copy link
Member

arporter commented Nov 5, 2024

Simon has reported that Sign2CodeTrans produces invalid code when applied to a SIGN that has an array as argument. One of the examples of this is from src/OCE/SBC/sbcblk_algo_andreas.F90 in NEMO from which I've constructed the following:

    program test_prog
      integer, parameter :: wp = kind(1.0d0)
      integer, parameter, dimension(0:4) :: A2D = (/1, 2, 3, 4, 5/)
      REAL(wp), DIMENSION(A2D(0)) :: ztmp1
      ztmp1 = 0.0
      ztmp1 = SIGN( MAX(ABS(ztmp1),1.E-6_wp), ztmp1 )
    end program test_prog'''
@arporter arporter added bug Release Planning and creating PSyclone releases labels Nov 5, 2024
@arporter
Copy link
Member Author

arporter commented Nov 5, 2024

The code PSyclone currently generates for this is:

  tmp_abs = MAX(ABS(ztmp1), 1.e-6_wp)
  if (tmp_abs > 0.0) then
    res_abs = tmp_abs
  else
    res_abs = tmp_abs * -1.0
  end if
  res_sign = res_abs
  tmp_sign = ztmp1
  if (tmp_sign < 0.0) then
    res_sign = res_sign * -1.0
  end if
  ztmp1 = res_sign

and the problem is that tmp_abs is declared as a scalar but MAX(ABS(ztmp1), 1.0E-6) is an array.

@arporter
Copy link
Member Author

arporter commented Nov 5, 2024

What we should generate is:

do i in shape
  tmp_abs = MAX(ABS(ztmp1(i)), 1.e-6)
  if (tmp_abs > 0.0) then
    res_abs = tmp_abs
  else
    res_abs = tmp_abs * -1.0
  end if
  res_sign = res_abs
  tmp_sign = ztmp1(i)
  ...
  ztmp1(i) = res_sign
end do

@arporter
Copy link
Member Author

arporter commented Nov 5, 2024

Alternatively, could we apply Reference2ArrayRangeTrans to

  ztmp1 = SIGN( MAX(ABS(ztmp1),1.E-6_wp), ztmp1 )

to get:

  ztmp1(:) = SIGN( MAX(ABS(ztmp1(:)),1.E-6_wp), ztmp1(:) )

and then ArrayAssignment2LoopsTrans to get:

  do i = 1, ...
     ztmp1(i) = SIGN(MAX(ABS(ztmp1(i)), 1.E-6_wp), ztmp1(i))
  end do

At which point, the Sign2CodeTransformation should work OK?

@arporter
Copy link
Member Author

arporter commented Nov 5, 2024

The transformation itself had a comment saying we assume that the argument is a real scalar because we can't query the datatype (with a reference to an old issue). I've closed that old issue (#658) and am proceeding to do better. However, I immediately hit the fact that we don't know what the return type of MAX or ABS is and so the datatype of the argument to SIGN comes back as UnresolvedType. We hit this situation because we can't get the datatype of MAX and ABS as they are Calls.

arporter added a commit that referenced this issue Nov 6, 2024
arporter added a commit that referenced this issue Nov 6, 2024
arporter added a commit that referenced this issue Nov 6, 2024
@arporter
Copy link
Member Author

arporter commented Nov 6, 2024

It turns out that ABS2CodeTrans made the same assumption about operating on a scalar, real argument so I've generalised my fix (validation really) so that it is used there too.

arporter added a commit that referenced this issue Nov 7, 2024
arporter added a commit that referenced this issue Nov 7, 2024
arporter added a commit that referenced this issue Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug in progress Release Planning and creating PSyclone releases
Projects
None yet
Development

No branches or pull requests

1 participant