Skip to content

Conversation

@henryleberre
Copy link
Member

My apologies for the rather large PR. This removes ~400 LOC while adding features. Here is an attempt at summarizing my changes.

Cray / CCE

Introduce support for the compilation of pre_process and simulation with the Cray Fortran compiler (ftn) for CPUs with limited GPU support. The GPU version of simulation doesn't work yet because of a compiler bug. I am waiting on the vendor to fix this issue. Most GPU-related changes/fixes are included in this Pull Request. These include:

  • Ensuring HDF5 and FFTW3 are not configured/built by the CMake superbuild file for dependencies when Cray is the target compiler. We create dummy CMake targets in this case.
  • Adding the correct flags for compilation with Cray. Since ACC offloading is enabled by default, we supply -h noacc -x acc when targeting CPUs and/or building a target other than simulation.
  • Creating a macros.fpp file to seamlessly handle the allocation of GPU arrays with a single call.
  • Adding an entry to mfc.sh load for Crusher.

mfc.sh

  • We now keep track and install Python dependencies into the virtualenv in a far simpler and more efficient manner.
  • Added mfc.sh cloc.

Toolchain/

Refactor (for a final time) the toolchain code:

  • modes have been replaced with simpler and more versatile switches: --[no-]mpi, --[no-]gpu, and --[no-]debug. These are options that can be turned on and off independently.
  • We properly handle cases where CMake configuration has failed or was incomplete. We do so by checking for the existence of a CMakeCache.txt file.
  • Removed the user .yml configuration file.
  • Parsing of template files has been improved and simplified. Expressions inside {} are now directly passed to eval() with special access to the dictionary of parsed command-line arguments.
  • References to member classes within the global MFCState are no longer. Modules are now publicly available and export their functionality to others.

Website / Documentation

  • MFC's website and documentation have been updated for a smoother user onboarding experience. Note: We now have to build and install the most recent release of Doxygen for the documentation to render properly.
  • I also added bibtex syntax highlighting to the MFC paper citation.

CMake

  • case.fpp files are gone from all targets except simulation.
  • CMakeLists.txt handles src/common code more carefully:
    • src/common code is now preprocessed into src/<target>/autogen where <target> is the name of the current target being compiled. This fixes bugs that resulted from different targets updating the same (common) src/common/autogen/*.f90 source files and causing undesirable recompilations.
    • src/common is now part of the Fypp include path.

Windows

I updated the mfc.bat file to work better!

@henryleberre henryleberre added documentation Improvements or additions to documentation enhancement New feature or request labels Jan 12, 2023
@sbryngelson
Copy link
Member

Thanks, @henryleberre ! I have some concerns about the use of @:ALLOCATE everywhere for a few reasons:

  1. It looks like it automatically does a enter data create on the GPU. Also, what about edge cases where you don't want to do a create right after an allocation for GPU? Do we know if any such cases exist or could exist?
  2. This might be good for many cases, but it obscures what's really going on for the developer.
    Can you clarify these points for me?

I'm also somewhat concerned about our growing use of fypp. I see that fypp hasn't had any maintenance or commits in > 1yr. If fypp is abandoned, what is the backup plan?

  • We can obviously fork it and manage it ourselves, but I don't want to do this.
  • It should keep working so long as the required python version remains available. But as newer python versions are released, this could eventually cause a problem. It looks like python 3.11 and 3.12 are already causing breaking changes for other packages.

On an entirely separate note, you replaced the banner with one with no background. This might cause readability issues for folks using white backgrounds.

@sbryngelson sbryngelson self-assigned this Jan 12, 2023
@sbryngelson
Copy link
Member

It also looks like the website has a big gray box in the middle if I'm not mistaken.

@henryleberre
Copy link
Member Author

Here are my thoughts, @sbryngelson !

@:ALLOCATE & @:DEALLOCATE

Use case

The macros are only intended to be used when you want to:

allocate(...)
!$acc enter data create(...)

and

deallocate(...)
!$acc exit data delete(...)

a variable. @:ALLOCATE isn't very descriptive, however, but @:ALLOCATE_ON_CPU_AND_GPU_IF_GPU_BUILD() would be too long! Can you think of a better name?

The other, regular allocate() and deallocate() calls remain.

Purpose

  1. DRY: It is more concise and leads to less errors since the ... part of the statement becomes DRY-compliant. You can't forget to update array bounds or add/remove a new variable on both lines. I decided to create these two macros after failing to properly add the directives myself.

  2. Compiler Support: Both GNU Fortran and Cray Fortran require you to !$acc enter data create(...). The top-level !$acc declare create is only used to propagate the variable's scope to that of the entire program.

    [According to a comment on a stack overflow post by an NVIDIA representative (OP of the accepted answer)] NVFortran's Fortran runtime's implementation of allocate can recognize when variables are !$acc declare create'd and perform the GPU allocation without the need to explicitly do so in code. This appears to be separate from their 'unified memory' model.

    The feature mentioned above may have made its way into the OpenACC standard since then. During the 'Cray Office Hours' I attended, I had raised this question, but I don't think anyone had a definitive answer. They should be looking into it.

    TL;DR: With all of this in mind, the use of !$acc <enter/exit> data <create/delete> is currently the only way forward to support a more diverse set of compilers, some of which supporting offloading to GPUs from other vendors.

Fypp, a liability?

I understand your concern. This happens to a lot of projects. I haven't encountered a bug myself with Fypp - ever.. but that can change.

  1. If worse comes to worst, we can replace the .fpp files with the .f90 files in src/<target>/autogen. We would of course no longer enjoy the benefits of using Fypp, but we could easily live without them. Modules would be longer, less DRY code, and we would have to rely on the C preprocessor for case optimization.

  2. Fypp appears to be quite popular in the Fortran community. If it is ever in critical need for repair(s), I have no doubt that a community version would be released and took precedence over the original.

As a result, I don't think that Fypp is currently a liability for us. Do you agree? If not, should we consider reverting to .f90 files?

Website

Yes, my bad! There was supposed to be more content so the main content section between the header and the footer is white. Internal padding/margins make it visible. I can remove it.

MFC Logo (README)

Indeed! I systematically use dark mode for more reasons than just personal taste so I never have it off. I had replaced it while updating the one on the website. I will revert it.

Looking at it now, I do somewhat prefer this version of the logo in dark mode with the transparent background. Adding more definition to the edges by inserting a black outline could be a fix.

@sbryngelson
Copy link
Member

Agreed - make the changes for the website and logo and then I will pull. Thanks!

@sbryngelson sbryngelson merged commit 597568b into MFlowCode:master Jan 13, 2023
@sbryngelson sbryngelson mentioned this pull request Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

2 participants