Request to add toolchain for NEC #53

PouyanLaleh · 2023-05-25T15:03:14Z

Hello,

The toolchain for cross-compiling on the NEC system is added. The READM file is modified to instruct compiling for NEC as well. We applied minor changes in 2 cmake files to avoid compilation errors on NEC. With this pull request, nothing is modified in the code.

Thanks,
Laleh

FussyDuck · 2023-05-25T15:03:20Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Fatemeh Pouyan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

mlange05 · 2023-05-30T13:37:57Z

Hi, first of all, many thanks for this contribution; this looks like a very valuable addition to our collection of architectures, and the numbers seem very impressive. We have no facilities right now to automatically test this, but we would very happily accept the environment as a reference implementation.

For the contribution itself, I've flagged a few inconsistencies, that simply seem like debug/dev leftovers; but it looks like a small clean-up effort. I've also noted that the base and target branch are main, while we tend to merge new contributions into develop first. I'll change the target to develop, but you might also want to consider a rebase or merge to make the contribution more up-to-date?

@PouyanLaleh You'll also need to sing the CLA (fussyduck). I'll put this into "changes requested" for now, but once the above is addressed and the license is signed, I'll enable the CI runners to get this merged quickly. And thanks again for a fantastic first contribution. 😄

mlange05

This looks great, many thanks. I've flagged a few things that need cleaning up before merging, and we will unfortunately be able to auto-test this, but once the clean-ups are addressed we should be able to merge this.

mlange05 · 2023-05-30T13:23:07Z

arch/ecmwf/aurora/nec/4.0.0/toolchain.cmake

+# OpenAcc FLAGS
+####################################################################
+
+set( OpenACC_Fortran_FLAGS "-acc -ta=tesla:lineinfo,deepcopy,maxregcount:100,fastmath" )


These look suspiciously like Nvidia GPU flags; does NEC really support OpenACC, or are these development/debug leftovers?

Sorry, they are leftovers that I forgot to remove them.

mlange05 · 2023-05-30T13:23:14Z

arch/ecmwf/aurora/nec/4.0.0/toolchain.cmake

+# OpenAcc FLAGS
+####################################################################
+
+set( OpenACC_Fortran_FLAGS "-acc -ta=tesla:lineinfo,deepcopy,maxregcount:100,fastmath" )


These look suspiciously like Nvidia GPU flags; does NEC really support OpenACC, or are these development/debug leftovers?

No, it doesn't. Leftovers as well!

mlange05 · 2023-05-30T13:24:17Z

arch/ecmwf/aurora/nec/4.0.0/toolchain.cmake

+set(ECBUILD_Fortran_FLAGS "${ECBUILD_Fortran_FLAGS} -floop-unroll-complete=200 ")
+set(ECBUILD_Fortran_FLAGS "${ECBUILD_Fortran_FLAGS} -ftrace")
+###set(ECBUILD_Fortran_FLAGS "${ECBUILD_Fortran_FLAGS} -fmove-loop-invariants-if ")
+###set(ECBUILD_Fortran_FLAGS "${ECBUILD_Fortran_FLAGS} -freplace-loop-equation ")


I don't mind having commented out options like this lying around, but could we maybe group them, and or annotate them with comments that provide a bit of context as to why they are enabled/disabled?

Yes, we can comment them because -ftrace enables profiling feature and -floop-unroll-complete=200 are used for optimisation purpose here.

mlange05 · 2023-05-30T13:25:07Z

arch/toolchains/ecmwf-hpc2020-nvhpc.cmake

@@ -37,14 +37,6 @@ set( OpenACC_Fortran_FLAGS "-acc=gpu -mp=gpu -gpu=cc80,lineinfo,fastmath" CACHE
 # Enable this to get more detailed compiler output
 # set( OpenACC_Fortran_FLAGS "${OpenACC_Fortran_FLAGS} -Minfo" )

-####################################################################


Again, not sure the NEC changes require us to change Nvidia configurations as such?

No, defining the OpenACC flags does not cause error on NEC but they are unnecessary for this toolchain.

mlange05 · 2023-05-30T13:26:45Z

arch/ecmwf/aurora/nec/4.0.0/toolchain.cmake

+# COMPILER
+####################################################################
+
+include( /opt/nec/ve/share/cmake/toolchainVE.cmake )


Traditionally, the low-level toolchains (per compiler version) are symlink up to the arch/toolchains/<arch-toolchain>-.cmake files higher up, for re-use?

mlange05 · 2023-05-30T13:27:30Z

README.md

@@ -60,29 +60,6 @@ Balthasar Reuter ([email protected])
  move parameter structures to constant memory. To enable this variant,
  a suitable CUDA installation is required and the `--with-cuda` flag
  needs to be passed at the build stage.
- **dwarf-cloudsc-gpu-scc-cuf-k-caching**: GPU-enabled and further


I think this here was deleted by accident? Or was this intended?

It'd also be great, if you could add a small section (or bullet point) to the README to tell users about the availability of the NEC variant. Probably a small paragraph?

Sorry, the deleted part was not intentional.

I thought I already added how to compile on NEC in the README file. I'll check that again.

reuterbal

Many thanks, it's great to have the NEC toolchain with the promising performance numbers in the repo. Can only parrot what Michael has said already, but left a few extra questions.

Just a remark on the CLA: You may have to add the email address that you used for the commits to your Github account to be able to sign the CLA.

reuterbal · 2023-05-30T13:33:36Z

CMakeLists.txt

@@ -19,7 +19,7 @@ project( dwarf-p-cloudsc LANGUAGES C Fortran )

 include( cmake/compat.cmake )
 if( CMAKE_Fortran_COMPILER_ID MATCHES "GNU")
-  ecbuild_add_fortran_flags("-ffree-line-length-none")
+	# ecbuild_add_fortran_flags("-ffree-line-length-none")


This should only affect the GNU compiler, does this really have to be removed for NEC? If it makes problems with the NEC compiler, could this be suitably guarded so it still takes effect for GNU?

Yes, it makes compiling error on NEC.

reuterbal · 2023-05-30T13:35:19Z

README.md

@@ -231,6 +208,17 @@ cd build
 ./bin/dwarf-cloudsc-fortran 4 16384 32   # The cleaned-up Fortran
 ./bin/dwarf-cloudsc-c 4 16384 32   # The standalone C version
 ```
+### Building on NEC SX-AURORA TSUBAS


Is the VE not called TSUBAS A ? Applies also to the line below.

No, VE is vector engine and NEC SX-AURORA TSUBAS is typically used as the architecture.

reuterbal · 2023-05-30T13:35:33Z

README.md

+HDF5_ROOT=HDF5-installation-PATH ./cloudsc-bundle build  --arch arch/ecmwf/aurora/nec/4.0.0/  [--single-precision] [--with-mpi] --hdf5 ON --cloudsc-fortran ON --cloudsc-prototype1 OFF --verbose --log DEBUG  
+```
+
+Currently available `NEC ompiler/version` selections are:


"compiler" is missing a "c"

reuterbal · 2023-05-30T13:36:01Z

README.md

@@ -272,27 +260,6 @@ srun bash -c "CUDA_VISIBLE_DEVICES=\$SLURM_LOCALID bin/dwarf-cloudsc-gpu-scc-hoi

 In principle, the same should work for multi-node execution (`-N 2`, `-N 4` etc.) once interconnect issues are resolved.

-### GPU runs: Timing device kernels and data transfers


Please re-add this paragraph

reuterbal · 2023-05-30T13:36:34Z

README.md

+
+Currently available `NEC ompiler/version` selections are:
+
+* `nec/4.0.0 (nfort, ncc, nc++)`


Could you add also an example invocation and approximate performance that can be expected?

Yes, I'll add it

reuterbal · 2023-05-30T13:37:12Z

arch/ecmwf/aurora/nec/4.0.0/env.sh

+export VE_FPE_ENABLE=DIV,INV,FOF,FUF,INE
+
+
+export PATH="/local/hdd/nabr/openmpi/nvhpc-nompi/20.9/bin:$PATH"


I don't think this absolute path setting should be necessary - also, it looks like it is carried over from volta?

No, I forgot to remove it

reuterbal · 2023-05-30T14:00:56Z

arch/ecmwf/aurora/nec/4.0.0/toolchain.cmake

+####################################################################
+# Enviroment Variables
+####################################################################
+set(NMPI_ROOT /opt/nec/ve/mpi/2.23.0)


Is this the same on all NEC VE machines, or should this maybe be exported as an environment variable in env.sh and then picked up in the toolchain file?

No, it's not. I think environment variable is the better idea for it.

reuterbal · 2023-05-30T14:02:37Z

src/cloudsc_fortran/CMakeLists.txt

        DEFINITIONS ${CLOUDSC_DEFINITIONS}
    )

+    target_link_libraries( dwarf-cloudsc-fortran PRIVATE cloudsc-common-lib )


Is there any reason for adding target_link_libraries, target_include_directories separately instead of pointing here to the cloudsc-common-lib in LIBS, as it was done before? That way, include and link settings propagate automatically.

I need to check this part.

Fatemeh Pouyan added 2 commits May 25, 2023 16:22

CMake Toolchain for NEC

722d0d1

Instruction for compiling is added

0d6281f

mlange05 changed the base branch from main to develop May 30, 2023 13:38

mlange05 requested changes May 30, 2023

View reviewed changes

reuterbal requested changes May 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Request to add toolchain for NEC #53

Request to add toolchain for NEC #53

PouyanLaleh commented May 25, 2023

FussyDuck commented May 25, 2023

mlange05 commented May 30, 2023

mlange05 left a comment

mlange05 May 30, 2023

PouyanLaleh May 30, 2023

mlange05 May 30, 2023

PouyanLaleh May 30, 2023

mlange05 May 30, 2023

PouyanLaleh May 30, 2023

mlange05 May 30, 2023

PouyanLaleh May 30, 2023

mlange05 May 30, 2023

mlange05 May 30, 2023

mlange05 May 30, 2023

PouyanLaleh May 30, 2023

reuterbal left a comment

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

reuterbal May 30, 2023

PouyanLaleh May 30, 2023

		@@ -272,27 +260,6 @@ srun bash -c "CUDA_VISIBLE_DEVICES=\$SLURM_LOCALID bin/dwarf-cloudsc-gpu-scc-hoi

		In principle, the same should work for multi-node execution (`-N 2`, `-N 4` etc.) once interconnect issues are resolved.

		### GPU runs: Timing device kernels and data transfers


		Currently available `NEC ompiler/version` selections are:

		* `nec/4.0.0 (nfort, ncc, nc++)`

		export VE_FPE_ENABLE=DIV,INV,FOF,FUF,INE


		export PATH="/local/hdd/nabr/openmpi/nvhpc-nompi/20.9/bin:$PATH"

Request to add toolchain for NEC #53

Are you sure you want to change the base?

Request to add toolchain for NEC #53

Conversation

PouyanLaleh commented May 25, 2023

FussyDuck commented May 25, 2023

mlange05 commented May 30, 2023

mlange05 left a 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

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

reuterbal left a 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

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