-
Notifications
You must be signed in to change notification settings - Fork 112
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
Carsten/comm proxy #979
base: master
Are you sure you want to change the base?
Carsten/comm proxy #979
Conversation
Signed-off-by: Carsten Uphoff <carsten.uphoff@intel.com>
f8ca99a
to
3e47ac5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, sorry for the delay; partial review for now.
Also, why not use the SeisSol Device module instead of "just" SYCL allocation? It should have all functions available there already—if not, then we can add them there.
cmake_minimum_required(VERSION 3.13) | ||
project(SeisSol-comm-proxy LANGUAGES CXX) | ||
|
||
option(SYCL "Use SYCL to allocate buffers" OFF) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make that compile automatically into the program? Also, maybe check for ACL_DEVICE
?
|
||
namespace seissol { | ||
|
||
Args ArgParser::parseArgs(int argc, char** argv) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use utils/args.h
?
@@ -0,0 +1,394 @@ | |||
/* Generated by re2c 2.2 on Thu Oct 19 07:04:58 2023 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it's generated code—I'm not a huge fan of GOTOs. But ok. Btw why not YAML? Or use a submodule for a Json reader? Is that too big/small? If so, then why not Hdf5?
|
||
namespace seissol { | ||
|
||
class SyclAllocator : public Allocator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only SYCL? Why not use the Device submodule and make it available for all architectures? The module should support that as well—and it may be closer to the truth when simulating, as SeisSol also uses the Device submodule for everything related to wavepropagation.
@@ -1,12 +1,13 @@ | |||
#include <Parallel/MPI.h> | |||
#include "Solver/time_stepping/AbstractGhostTimeCluster.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have these been formatted? Then maybe add them to the formatting list?
I was looking for a bug related to communication and I thought it would be useful to have a reproducer for the communication part only. I added a reproducer that currently only checks whether communication "works", but I think one could easily extend it to analyze communication performance on a large number of nodes.
In order to use it one has to run SeisSol with "DumpLocalTimeSteppingStructure=1" in the Output section. SeisSol then writes a JSON dump of the MeshStructure and TimeStepping structs. The JSON file is then read into the communication proxy and the MeshStructure thingy is reconstructed. I then re-use the Actor classes in order to orchestrate the communication.