search

E3SM-Project / E3SM

Energy Exascale Earth System Model source code. NOTE: use "maint" branches for your work. Head of master is not validated.
https://docs.e3sm.org/E3SM
Other
353 stars 365 forks source link

MOSART_sediment_mod issues with Cray compiler on Crusher #5677

Closed sarats closed 1 year ago

sarats commented 1 year ago

Encountered during recent tests.

 1590 ftn-855 ftn: ERROR MOSART_SEDIMENT_MOD, File = ../../../lustre/orion/cli115/proj-shared/testing/E3SM/components/mosart/src/riverroute/MOSART_sediment_mod.F90, Line = 2, Column = 8
 1591   The compiler has detected errors in module "MOSART_SEDIMENT_MOD".  No module information file will be created for this module.
 1592
 1593
 1594 ftn-113 ftn: ERROR MAINCHANNELSEDIMENT, File = ../../../lustre/orion/cli115/proj-shared/testing/E3SM/components/mosart/src/riverroute/MOSART_sediment_mod.F90, Line = 294, Column = 255
 1595   IMPLICIT NONE is specified in the local scope, therefore an explicit type must be specified for data object "I".
 1596
 1597
 1598 ftn-1725 ftn: ERROR MAINCHANNELSEDIMENT, File = ../../../lustre/orion/cli115/proj-shared/testing/E3SM/components/mosart/src/riverroute/MOSART_sediment_mod.F90, Line = 294, Column = 256
 1599   Unexpected syntax while parsing the WRITE statement : ")" was expected but found "EOS".
sarats commented 1 year ago

cc @grnydawn @liho745 @bishtgautam

sarats commented 1 year ago

This seems to be the line with issue:

https://github.com/E3SM-Project/E3SM/blob/0f18ded2aeeacff52c47a0a36f8658824a4612d6/components/mosart/src/riverroute/MOSART_sediment_mod.F90#L294

sarats commented 1 year ago

This line is 261 characters which is longer than the 255 that was set for Cray compilers. This was the only instance in E3SM where this was exceeded.

sarats commented 1 year ago

Note: -N 255 is currently set for Cray. Don't know if we should switch to 1023 for the sake of this one file. Anyway, 1023 seems like a readability nightmare too.

Or we can switch this to a free-format source file with no restrictions.

Relevant man page excerpt: 

   -N col Specifies the line width for fixed-format source lines.  The value used for col specifies the maximum number of columns per line.

              ? For fixed form sources, col can be set to 72, 80, 132, 255, or 1023.

              ? Characters in columns beyond the col specification are ignored.

              ? By default, lines are 72 characters wide for fixed-format sources.

              ? There is no line size limit for free-format source files.
rljacob commented 1 year ago

We should always use free format.

sarats commented 1 year ago

Then is the fix just a case of changing the file extension?

rljacob commented 1 year ago

No its finding the cray compiler flag for free format and replace "-N 255" with that.

sarats commented 1 year ago

Are you sure all E3SM sources are free format? The Cray compiler guesses from the file extension IIRC.

sarats commented 1 year ago

Anyway, other compilers like GNU have blocks like

 39 string(APPEND FIXEDFLAGS " -ffixed-form")
 40 string(APPEND FREEFLAGS " -ffree-form")
rljacob commented 1 year ago

That's also a mistake. We should use free-format everywhere.

If its guessing from the extension, .F90 should imply free-format while .F implies fixed form.

philipwjones commented 1 year ago

Cray is -f free Note that MPAS has free form and unfortunately used .F extensions so we can't rely on suffix alone

sarats commented 1 year ago

Yes. The above Cray option was a legacy option carried over from previous incarnations of Cray machines. We can just switch to free for all ;) and see what happens.

sarats commented 1 year ago

Still on a tangent, do we have a code style preference on line length?

sarats commented 1 year ago

In fact, the Cray compiler block for Crusher/Frontier actually sets it to free format.

string(APPEND FFLAGS " -f free -N 255 -h byteswapio -em")

@grnydawn Do you recall why -N 255 was added?

sarats commented 1 year ago

@twhite-cray When both of the following options are specified for Cray Fortran, -f free -N 255 - is it expected to interpret the MOSART_sediment_mod.F90 as a fixed format file? Does the second option override the first?

trey-ornl commented 1 year ago

The -N option should not affect whether the compiler expects free or fixed formats. The suffix .F90 tells the compiler to default to free and run it through the C preprocessor. The -f free is redundant in this case.

trey-ornl commented 1 year ago

I feel like -N used to apply to free format as well as fixed, but it doesn't now.

grnydawn commented 1 year ago

@sarats , "the -N 255" option was inherited from "cray.cmake" in cmake_macros. When I created an entry for Spock in machinefile, I copied the options to Spock entry.

sarats commented 1 year ago

@twhite-cray I'm confused, should the Cray compiler complain about the line length in this case then (free format file)?

trey-ornl commented 1 year ago

Sorry, I didn't read the whole thread.

I reproduced the issue with a simple free-format code. Despite what the man page says, it appears that -N applies to both fixed and free formats. In other words, it chops off free-format files too.

I'll submit a bug report.

If everything is free format, you should be able to just eliminate -N.

trey-ornl commented 1 year ago

I created a ticket for the HPE Cray compiler. Is there a fix you would prefer?

grnydawn commented 1 year ago

Removing "-N 255" flag eliminates the cray compiler error during MOSART_sediment_mod.F90 compilation on Frontier. I am running e3sm_integration test on Frontier to see if the change introduces any new issue.

sarats commented 1 year ago

-N continues to apply to all formats, and we fix the man page.

Don't have a strong preference but I guess fixing the man page makes sense to avoid confusion if someone is explicitly using -N.

sarats commented 1 year ago

https://github.com/E3SM-Project/E3SM/pull/5444 removed the -N 255 flag.

Orthogonally, we should think if adding a code style recommendation for line length along with pre-commit hook would be beneficial to ensure readability.