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

NAG fixes. #213

Merged
merged 2 commits into from
Jul 15, 2024
Merged

NAG fixes. #213

merged 2 commits into from
Jul 15, 2024

Conversation

DJDavies2
Copy link
Contributor

Resolves #212. There are three types of change:

Addition of target attribute to some variables (see e.g. ecmwf/fckit#43)

In array_c_to_f (in atlas_Field_module.fypp) the call to c_f_pointer is sometimes passing a shape where one of the bounds is 0. This results in tmp being null, meaning that subsequent array slices being pointed at by array_fptr are invalid. I have addressed this by allocated a 0 size array.

atlas_fctest_field_wrap was failing like this:

/spice/scratch/frwd/cylc-run/mi-be984/work/1/git_clone_atlas/atlas/src/tests/field/fctest_field_wrap.F90:116: warning: FCTEST_CHECK_EQUAL(data(i,j,l) , real(1000i+100j+10*k+l,c_double) )
--> [ 1.1210000000000000E+03 != 1.1120000000000000E+03 ]

I tracked this down to the function array_strides which was returning the wrong values, resulting ultimately in the arrays being compared in the test being out of sync with each other. The difference between NAG and other compilers appears to be that NAG is pass the argument to array_strides via copy-in/copy-out; this can be verified by printing pointer values. This collapses the strides so that the array_strides calculation is incorrect.

I don't think array_strides can be fixed; what it is trying to do is unportable because passing arguments via copy-in/copy-out or by reference (or any other mechanism) is allowed. There is never any guarantee that this will give the right answer. The change here replaces the array_strides call with an inline implementation for the particular case that fails, but of course array_strides is used in many other situations (including outside of atlas) and I haven't touched those.

I really think that array_strides should be looked at again and rethought. As it stands it isn't just a portability issue across compilers, but also in the future as well; updated versions of compilers that currently work can start to fail this due to changes of implementation.

else
stridesf = 0
endif
#:else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no issue with rank == 2 ? Perhaps not in what gets tested...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't show up in a test. However it could just be that this isn't tested either in fckit or atlas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I extended this now with rank == {1,2,4} as well, to be sure.

@wdeconinck
Copy link
Member

Thanks once more in progressing with NAG.

Bullet 3. seems indeed to be quite problematic and not really a satisfactory solution...

If NAG is using copy-in (pass-by-value?) of 'data' instead of pass-by-reference, is that not detrimental for large arrays, or is there some introspection going on... ¯_(ツ)_/¯

Is there a means to force pass-by-reference in fckit's array_strides ?
Could you isolate a test in fckit (if you have time) to demonstrate the problem there as well, and perhaps a workaround for array_strides... If we were to review array_strides, is there a way to force arguments to be passed by reference instead of copy-in/copy-out?

Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving and merging already, but please consider above comments @DJDavies2

@wdeconinck wdeconinck merged commit b698b91 into ecmwf:develop Jul 15, 2024
141 of 142 checks passed
wdeconinck added a commit that referenced this pull request Jul 15, 2024
* hotfix/0.38.1:
  Update Changelog
  Version 0.38.1
  NAG fixes. (#213)
  Wrap out-of-bounds source cells into the domain (#211)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some issues with NAG compiler
2 participants