-
Notifications
You must be signed in to change notification settings - Fork 6
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
Reduce variables and critical markup #71
Conversation
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.
Overall LGTM. I was wondering, even if we agree that this would eventually go into a pre-processing step, is it worth to add any notes on the docs mentioning how to currently use the OMP port?
@@ -7,6 +7,9 @@ struct complex_t | |||
int imag; | |||
}; | |||
|
|||
// WARNING: custom reductions are not supported in a distributed Faasm |
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.
Are they not?
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.
Unfortunately only reversible functions are supported, so we can't accept arbitrary user-defined reduction operations.
A merge region works out what change has been made, then effectively replays it back on the main copy of the memory for that function. If the function that made the change is not reversible, this type of replay isn't possible.
#include <omp.h> | ||
|
||
#ifdef __wasm | ||
#include <faasm/shared_mem.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.
I know we discussed this would potentially take place in a pre-processing step but could we hide the definition (or nulling-out) of FAASM_REDUCE
in a header file?
Also, it is not clear to me why we check for the __wasm
macro here and not in the other functions modified in this PR.
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.
This check doesn't actually need to be here so I'll remove it. However, this pattern is required for any code we want to compile natively, otherwise the Faasm macros will cause compiler errors.
I'm not sure which header we could put this in to hide it. We can't/ don't want to assume any Faasm-specific headers will be present in the native build, so the only way we could hide things is doing a fully transparent port of the OpenMP pragmas (so that native code would be completely unaware of Faasm).
int nThreads = 100; | ||
int counter = 20; | ||
|
||
#pragma omp parallel num_threads(nThreads) default(none) shared(counter) |
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.
I am not sure I understand what this test is doing. If we are testing the atomic pragma, I would expect to see the
#pragma omp atomic
construct.
I assume this has to do with the NOTE
beneath, but I still don't really get it.
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.
If we were compiling to native code we would have #pragma omp atomic
, but this doesn't actually result in anything we can hook into in Faasm, so we have to use the custom Faasm macro.
I think the examples and experiments are probably sufficient documentation. Anything in markdown would rot fairly quickly as it's all changing quite fast. |
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.
👍
Although we can infer a lot about a multi-threaded application from APIs like OpenMP at runtime, they don't quite provide enough information.
This PR introduces a few explicit host interface calls that tell Faasm which variables are reduce variables, and where to acquire and release local locks. It's possible that we can perform either a preprocessing step, or worst case modify OpenMP, to insert these calls at compile time, and simplify the inference at runtime.
For now we will just inline the calls to check this can work, and the preprocessing step or modifying upstream toolchains can come later.
I've also removed a few unused/ unsupported OpenMP functions that were clogging things up.