Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added vectorized Riemann solvers for the single-layer shallow water equations #133

Merged
merged 2 commits into from
May 26, 2018

Conversation

chaulio
Copy link
Contributor

@chaulio chaulio commented Apr 10, 2018

These changes introduce new vectorized implementations for the single-layer shallow water equations (chile2010 example). The user can easily switch between the serial and the vectorized code: to use vectorization, one needs to define GEOCLAW_VEC=1 before compiling. More info in the chile2010 Makefile.

hvL=qr(i-1,nv)
hvR=ql(i,nv)

sw1 = 0.0d0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to add a comment as to why you needed to decompose this into scalars for future reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

integer meqn,mwaves,maxiter
!double precision fw(meqn,mwaves)
!double precision sw(mwaves)
double precision sw1,sw2,sw3,fw11,fw12,fw13,fw21,fw22,fw23,fw31,fw32,fw33
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have been using real(kind=8) instead of double precision with the intent to perhaps support multiple precisions depending on the case. I know the former is less portable but I think it's probably best to be consistent unless there is another reason to use double precision.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now replaced double precision with real(kind=8) in my code. Note however that the original geoclaw_riemann_utils.f still uses double precision, you may want to check whether this should also be modified.

use amr_module, only: mcapa

use storm_module, only: pressure_forcing, pressure_index
use statistics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a module I created to measure the execution time of each solver, but I removed it before this pull request. Apparently, I forgot to delete it from a couple of files, and my compilation tests did not complain because the .mod file was still present in my computer. I have already removed these "use statistics" lines from the code.

@rjleveque
Copy link
Member

When I test this, I seem to be missing the module needed by

    use statistics

…equations:

* double precision -> real(kind=8) ;
* !DIR$ SIMD -> !$OMP SIMD ;
* Added a few comments explaining that I had to decompose some arrays into several scalars because of what seems to be a bug with the Intel Compiler;
* Removed subroutines riemann_ssqfwave and riemann_fwave from geoclaw_riemann_utils_vec.f90 because they were not being used and also were not compatible with the vectorized code (different order in the array indices);
* Removed "use statistics", which is a module I was using to measure the execution time taken by each solver.
@chaulio
Copy link
Contributor Author

chaulio commented Apr 11, 2018

I have pushed a new commit that addresses the issues you have pointed out.

Main changes:

  • double precision -> real(kind=8) ;
  • !DIR$ SIMD -> !$OMP SIMD ;
  • Added a few comments explaining that I had to decompose some arrays into several scalars because of what seems to be a bug with the Intel Compiler;
  • Removed subroutines riemann_ssqfwave and riemann_fwave from geoclaw_riemann_utils_vec.f90 because they were not being used and also were not compatible with the vectorized code (different order in the array indices);
  • Removed "use statistics", which is a module I was using to measure the execution time taken by each solver.

@mandli
Copy link
Member

mandli commented Apr 11, 2018

This looks good to me now. Randy can you test this quick and make sure it also works for you?

@rjleveque
Copy link
Member

It compiles and runs, but seems to be slower. See the comment I posted on clawpack/geoclaw#291.

@mandli mandli merged commit a8712d6 into clawpack:master May 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants