Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) #44

piotrows · 2023-11-17T15:06:07Z

Aiming at the larger changes incoming with the CP/DP PR, this PR performs some tidy-up:

in the DP and SP BLAS calls, ensures that explicit constant values are passed with JPRD and JPRM, respectively (instead of JPRB)
and removes compiler warnings through
commenting out unused statement function in tpm_pol
modifying write format in sugaw and benchmark that ensures W=>D+7
adding explicit, matching REAL type cast to JPRB in gpnorm invocation of max/min (this is due to the fact that one of the two arguments in max/min function is hard coded JPRD)
Code tested on ATOS with gcc,intel,nvhpc and on the WSL workstation with gcc,intel,nvhpc, as well as clang/gfortran on Mac ARM. I've noticed that some tests are failing under Intel 2024 on WSL Ubuntu workstation.

FussyDuck · 2023-11-17T15:06:12Z

All committers have signed the CLA.

samhatfield · 2023-11-17T15:17:27Z

src/trans/internal/tpm_pol.F90

-DA(KKN,KKM)=SQRT( (REAL(2*KKN+1,JPRD)*REAL(KKN-KKM,JPRD)&
-                   &*REAL(KKN+KKM,JPRD))&
-                &/  REAL(2*KKN-1,JPRD) )
+!DA(KKN,KKM)=SQRT( (REAL(2*KKN+1,JPRD)*REAL(KKN-KKM,JPRD)&


Shall we just delete this? You're right, it doesn't seem to be used at all.

Do we wait for a second opinion or should I give it a go ?

I'm always nervous about modifying code that is almost as old as I am. Let's see what @marsdeno thinks.

It definitely looks pretty safe to me to be deleted!

samhatfield · 2023-11-17T15:22:06Z

Apart from my comment above, this looks ready for merging.

samhatfield · 2023-11-17T15:22:44Z

Some CI suites are failing but that looks like another problem...

marsdeno · 2023-11-20T15:41:21Z

src/trans/algor/butterfly_alg_mod.F90

Nothing to say on changes from JPRB to JPRM/JPRD, but while you're there, would it make sense to look at other precision-related things in this file? Things I see looking at file are

calls to REAL() conversion routine with JPRB kind request, when we know whether it should be JPRM or JPRD (for ex. l.890)

calls to NINT(xx,JPRB) look rather strange to me, shouldn't that be e.g. JPIB?

NINT(xx, JPRB) does not make sense to me, especially since assigned values (at least following are of integer(kind=JPIM) (always 32bit integer).

INTEGER(KIND=JPIM) :: M_ORDER =0 ! M of original matrix INTEGER(KIND=JPIM) :: N_ORDER =0 ! N of original matrix INTEGER(KIND=JPIM) :: N_CMAX =0 ! Max number of columns in each submatrix at level 0 INTEGER(KIND=JPIM) :: N_LEVELS =0 ! Max level in dyadic hierarchy INTEGER(KIND=JPIM) :: IBETALEN_MAX=0 ! Max workspace for one level of interim results "beta"

The REAL() call on line 890 seems to be correct because ZVECOUT is JPRB.

As for NINT, it seems to do the following currently:

1. Round real number to nearest integer, returning a real number. 2. Implicit type conversion to integer. 3. Assign to integer.

when indeed it could be shortened to:

1. Round real number to nearest integer, returning an integer. 2. Assign to integer.

Presumably replacing the JPRB with JPIB will fix this, as Olivier suggests.

Shall we leave it all for a separate PR, or we extend this one ?

Well unless I misunderstand @marsdeno's point about REAL() conversions, the only thing outstanding is the NINT calls. If you could change those to use JPIM with one more commit on this PR, I think we're ready for merging!

The REAL() call on line 890 seems to be correct because ZVECOUT is JPRB.

My comment here was rather that we know from the code that in this particular instance, JPRB is in fact single precision.

Well unless I misunderstand @marsdeno's point about REAL() conversions, the only thing outstanding is the NINT calls. If you could change those to use JPIM with one more commit on this PR, I think we're ready for merging!

The REAL() thing is not very important, I think, but it is similar in nature to the original point being addressed, regarding replacing JPRB by JPRM/JPRD in places where we know what it is.

I understand now. It's because on this line we are inside an IF branch which is only entered when LLDOUBLE = .FALSE.

@samhatfield @marsdeno this two issues are now corrected. We still have a type incompatibility in BLAS calls, e.g.
Users/napz/Codes/ectrans-typefix/src/trans/internal/ledir_mod.F90:170:53:

141 | CALL DGEMM('T','N',ILA,KIFC,KDGLU,1.0_JPRD,S%FA(KMLOC)%RPNMA,KDGLU,&
| 2
......
170 | CALL DGEMM('T','N',ILA,KIFC,KDGLU,1.0_JPRD,ZRPNMA,KDGLU,&
| 1
Warning: Type mismatch between actual argument at (1) and actual argument at (2) (REAL(8)/REAL(4)).
If I understood correctly, @marsdeno you are already correcting it elsewhere, so we leave it alone for now.

wdeconinck · 2023-11-27T15:38:34Z

I like all new changes since my last review except the last commit 629a65f

The types of ZBETA and ZVEC_OUT are declared with JPRB

REAL(KIND=JPRB),ALLOCATABLE :: ZBETA(:,:,:)

Even if not executed due to if (.NOT. LLDOUBLE) it is counterintuitive to have that different from the declaration.
and compiler will create conversion instructions.

samhatfield · 2023-11-27T15:42:38Z

I have no strong feelings on this one way or the other...

marsdeno · 2023-11-27T15:44:58Z

I like all new changes since my last review except the last commit 629a65f

The types of ZBETA and ZVEC_OUT are declared with JPRB
REAL(KIND=JPRB),ALLOCATABLE :: ZBETA(:,:,:)
Even if not executed due to if (.NOT. LLDOUBLE) it is counterintuitive to have that different from the declaration. and compiler will create conversion instructions.

Fair enough, I can see your point about counterintuitive, although I'm not sure about the situation with conversion instructions actually being any different.

wdeconinck · 2023-11-27T15:45:24Z

I have no strong feelings on this one way or the other...

Me neither really. Can stay as is in the PR if Olivier likes it.

piotrows · 2023-11-27T15:47:20Z

I like all new changes since my last review except the last commit 629a65f

The types of ZBETA and ZVEC_OUT are declared with JPRB
REAL(KIND=JPRB),ALLOCATABLE :: ZBETA(:,:,:)
Even if not executed due to if (.NOT. LLDOUBLE) it is counterintuitive to have that different from the declaration. and compiler will create conversion instructions.

Hmm, if I understand correctly, the conversion instructions would be generated in the inactive path only, and with the current (explicit: JPRM or JPRD) form the code seems better readable to me. @marsdeno @samhatfield ?
Edit: too late ;)
The only drawback I see is that perhaps the instruction cache may be negatively influenced if spurious conversion is actually generated. But this seems negligible.

piotrows · 2023-11-27T19:06:48Z

@wdeconinck @marsdeno @samhatfield just to make clear, I expect one of you to mercifully merge it if its ready.

samhatfield · 2023-11-29T09:44:57Z

@wdeconinck could I be given rights to merge pull requests?

… SP calls (instead of JPRB). Explicit type cast of JPRD to JPRB is added in gpnorm MIN/MAX calls to remove compilation warning of NVHPC

…tpm_pol commented out.

…JPRB=JPRM

wdeconinck · 2023-11-30T08:52:54Z

Thanks @piotrows for this cleanup!

marsdeno · 2023-11-30T08:54:15Z

[like] Olivier Marsden reacted to your message:

…

________________________________ From: Willem Deconinck ***@***.***> Sent: Thursday, November 30, 2023 8:53:05 AM To: ecmwf-ifs/ectrans ***@***.***> Cc: Olivier Marsden ***@***.***>; Mention ***@***.***> Subject: Re: [ecmwf-ifs/ectrans] Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) (PR #44) Thanks @piotrows<https://github.com/piotrows> for this cleanup! — Reply to this email directly, view it on GitHub<#44 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJ4E3JJCMYY43XZ4M3KF5JDYHBCPDAVCNFSM6AAAAAA7P5YCMWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZTGM2DCOJVGE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

piotrows requested review from wdeconinck, samhatfield and marsdeno November 17, 2023 15:10

samhatfield reviewed Nov 17, 2023

View reviewed changes

marsdeno reviewed Nov 20, 2023

View reviewed changes

wdeconinck approved these changes Nov 23, 2023

View reviewed changes

marsdeno approved these changes Nov 24, 2023

View reviewed changes

piotrows added 6 commits November 30, 2023 09:51

Constant numbers in BLAS calls are now JPRD for DP calls and JPRM for…

d9fd985

… SP calls (instead of JPRB). Explicit type cast of JPRD to JPRB is added in gpnorm MIN/MAX calls to remove compilation warning of NVHPC

Add missing JPRM imports

0114cff

Remove Intel compiler warnings: W=>D+7. Unused statement function in …

48f1537

…tpm_pol commented out.

Delete unused statement function

ced11aa

Fix NINT to use JPIM instead of JPRB in butterfly.

eacae7c

Fix real cast from JPRB to JPRM when we know thanks to LLDOUBLE that …

17e876f

…JPRB=JPRM

wdeconinck force-pushed the napz-blascalls-constantype-fix branch from 5513b86 to 17e876f Compare November 30, 2023 08:51

wdeconinck merged commit f83ba32 into develop Nov 30, 2023
21 checks passed

wdeconinck deleted the napz-blascalls-constantype-fix branch November 30, 2023 08:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) #44

Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) #44

piotrows commented Nov 17, 2023

FussyDuck commented Nov 17, 2023 •

edited

Loading

samhatfield Nov 17, 2023

piotrows Nov 17, 2023 •

edited

Loading

samhatfield Nov 17, 2023 •

edited

Loading

marsdeno Nov 20, 2023

piotrows Nov 24, 2023

samhatfield commented Nov 17, 2023

samhatfield commented Nov 17, 2023

marsdeno Nov 20, 2023

wdeconinck Nov 23, 2023

samhatfield Nov 24, 2023

samhatfield Nov 24, 2023

piotrows Nov 24, 2023

samhatfield Nov 24, 2023

marsdeno Nov 24, 2023

marsdeno Nov 24, 2023

samhatfield Nov 24, 2023

piotrows Nov 24, 2023

wdeconinck commented Nov 27, 2023

samhatfield commented Nov 27, 2023

marsdeno commented Nov 27, 2023

wdeconinck commented Nov 27, 2023 •

edited

Loading

piotrows commented Nov 27, 2023 •

edited

Loading

piotrows commented Nov 27, 2023

samhatfield commented Nov 29, 2023

wdeconinck commented Nov 30, 2023

marsdeno commented Nov 30, 2023 via email

Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) #44

Consistent definition of explicit constants in BLAS calls + warnings removal(explicit type conversion in gpnorm, format W=>D+7, unused statement function) #44

Conversation

piotrows commented Nov 17, 2023

FussyDuck commented Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

piotrows Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

samhatfield Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samhatfield commented Nov 17, 2023

samhatfield commented Nov 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wdeconinck commented Nov 27, 2023

samhatfield commented Nov 27, 2023

marsdeno commented Nov 27, 2023

wdeconinck commented Nov 27, 2023 • edited Loading

piotrows commented Nov 27, 2023 • edited Loading

piotrows commented Nov 27, 2023

samhatfield commented Nov 29, 2023

wdeconinck commented Nov 30, 2023

marsdeno commented Nov 30, 2023 via email

FussyDuck commented Nov 17, 2023 •

edited

Loading

piotrows Nov 17, 2023 •

edited

Loading

samhatfield Nov 17, 2023 •

edited

Loading

wdeconinck commented Nov 27, 2023 •

edited

Loading

piotrows commented Nov 27, 2023 •

edited

Loading