ecmwf-ifs / ectrans

Global spherical harmonics transforms library underpinning the IFS
Apache License 2.0
15 stars 30 forks source link

Compile failure for Intel #93

Closed DJDavies2 closed 2 months ago

DJDavies2 commented 2 months ago

I am getting this compile failure with ifort for the develop branch:

/home/h01/frwd/cylc-run/mo-bundle-build-base-latest/share/make_mo__spice_intel_debug/ectrans/src/trans/generated/ectrans_dp/internal/ftdir_ctl_mod.F90(147): error #5558: A pointer with the CONTIGUOUS attributes is being made to a non-contiguous target ZGTF => ZGTF_STACK(:,:) --^ compilation aborted for /home/h01/frwd/cylc-run/mo-bundle-build-base-latest/share/make_mo__spice_intel_debug/ectrans/src/trans/generated/ectrans_dp/internal/ftdir_ctl_mod.F90 (code 1) gmake[2]: [ectrans/src/trans/CMakeFiles/ectrans_dp.dir/generated/ectrans_dp/internal/ftdir_ctl_mod.F90.o] Error 1 gmake[2]: Waiting for unfinished jobs.... gmake[1]: [ectrans/src/trans/CMakeFiles/ectrans_dp.dir/all] Error 2 gmake[1]: Waiting for unfinished jobs....

samhatfield commented 2 months ago

Hi @DJDavies2 - which version of ifort are you using? I am using

ifort (IFORT) 2021.4.0 20210910

and I don't see this error.

samhatfield commented 2 months ago

You're right to raise this though. The documentation is pretty clear:

An array pointer with the CONTIGUOUS attribute can only be pointer associated with a contiguous target.

The CONTIGUOUS statements were introduced in commit 635f9bb. This commit contains changes imported from the IFS, and I think @RyadElKhatibMF wrote this originally. I noticed that ZGTF in FTDIR_CTL has CONTIGUOUS but not ZGTF in FTINV_CTL. So I wonder if we actually need CONTIGUOUS at all. Any thoughts @RyadElKhatibMF?

DJDavies2 commented 2 months ago

I am using Ifort 2019, a bit old unfortunately.

RyadElKhatibMF commented 2 months ago

Yes, I am used to signing my changes in the source code itself ;-) The bad idea in ectrans was to design interfaces with implicit declarations for arrays, allowing non-contiguous data arrays, while in practice we rarely use non-contiguous arrays. The consequence is that compilers will not make use of prefetching, and this will have a detrimental impact on the performance. We should use contiguous arrays as much as possible, either by the mean of CONTIGUOUS attribute (the easiest way, now) or even better by declaring arrays in the F77 style.

----- Météo-France ----- EL KHATIB Ryad DESR/CNRM/GMAP/ALGO @.*** Fixe : +33 561078466

De: "Sam Hatfield" @.> À: "ecmwf-ifs/ectrans" @.> Cc: "ryad el khatib" @.>, "Mention" @.> Envoyé: Mercredi 1 Mai 2024 16:05:58 Objet: Re: [ecmwf-ifs/ectrans] Compile failure for Intel (Issue #93)

You're right to raise this though. The [ https://www.ibm.com/docs/en/xffbg/121.141?topic=attributes-contiguous-fortran-2008 | documentation ] is pretty clear:

An array pointer with the CONTIGUOUS attribute can only be pointer associated with a contiguous target. The CONTIGUOUS statements were introduced in commit [ https://github.com/ecmwf-ifs/ectrans/commit/635f9bb1c36c40945077e8f7e1ca68649ad794e6 | 635f9bb ] . This commit contains changes imported from the IFS, and I think [ https://github.com/RyadElKhatibMF | @RyadElKhatibMF ] wrote this originally. I noticed that ZGTF in FTDIR_CTL has CONTIGUOUS but not ZGTF in FTINV_CTL . So I wonder if we actually need CONTIGUOUS at all. Any thoughts [ https://github.com/RyadElKhatibMF | @RyadElKhatibMF ] ?

— Reply to this email directly, [ https://github.com/ecmwf-ifs/ectrans/issues/93#issuecomment-2088514168 | view it on GitHub ] , or [ https://github.com/notifications/unsubscribe-auth/AYEAC4JGP5HSSPKYHWZ5Z7DZADZENAVCNFSM6AAAAABG4BMSCGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOBYGUYTIMJWHA | unsubscribe ] . You are receiving this because you were mentioned. Message ID: @.***>

samhatfield commented 2 months ago

Thanks for this Ryad. I realise I misunderstood the documentation.

An array pointer with the CONTIGUOUS attribute can only be pointer associated with a contiguous target. "Contiguous target" doesn't mean an array explicitly declared CONTIGUOUS - the target can be contiguous without this decorator of course.

Both targets in this case, ZGTF_STACK and ZGTF_HEAP, are contiguous already, looking at how their dimensions are declared. Hence I think the code is already correct as it is.

I also noticed some other people reporting compilation bugs for contiguous pointers in fairly recent versions of ifort. Therefore I think we have to declare this a bug in ifort 2019. @DJDavies2 is there no way you can update your ifort version? I would rather not add a preprocessing macro to remove CONTIGUOUS for earlier compiler versions.

DJDavies2 commented 2 months ago

I cannot update ifort myself, I don't have the system rights to do that. They will be updated at some point, but the timescale is months if not years as things stand.

If you don't want to have this sort of change that is completely understandable. I can keep these sorts of changes on a branch in my github repository if you would prefer to keep them off develop.

samhatfield commented 2 months ago

Fair enough. Yes, sorry about that. What you could do is run a simple find-and-replace across the source tree replacing , CONTIGUOUS with the empty string.

DJDavies2 commented 2 months ago

Okay, I will look into that. I will close this issue.