Skip to content

OpenMP support of tensor processing. #565

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.

Sign up for GitHub

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

Merged
highperformancecoder merged 7 commits into from
Oct 6, 2025
Merged

OpenMP support of tensor processing. #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
02515a3
OpenMP support of tensor processing.
highperformancecoder Sep 30, 2025
4923b4b
Merge branch 'master' into feature-enable-openmp
highperformancecoder Oct 4, 2025
5a77fe1
Fix regression test failures.
highperformancecoder Oct 5, 2025
db3757e
Separate serial/parallel handling of exceptions
highperformancecoder Oct 5, 2025
569489a
Address code review comments.
highperformancecoder Oct 5, 2025
6db0598
Merge branch 'feature-enable-openmp' of github.com:highperformancecod…
highperformancecoder Oct 5, 2025
fcf09c4
Merge branch 'master' into feature-enable-openmp
highperformancecoder Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,11 @@ ifeq ($(CPLUSPLUS),clang++)
FLAGS+=-Wno-unused-command-line-argument -Wno-unknown-warning-option -Wno-defaulted-function-deleted -Wno-uninitialized
endif

ifdef OPENMP
FLAGS+=-fopenmp
LIBS+=-fopenmp
endif

ifeq ($(DEBUG), 1)
FLAGS+=-Wp,-D_GLIBCXX_ASSERTIONS
endif
Expand Down
13 changes: 12 additions & 1 deletion engine/evalGodley.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,22 @@ namespace minsky

void EvalGodley::eval(double* sv, const double* fv) const
{
for (size_t i=0; i<initIdx.size(); ++i)
size_t initIdxSize=initIdx.size();
#ifdef _OPENMP
#pragma omp parallel for if(initIdxSize>20)
#endif
for (size_t i=0; i<initIdxSize; ++i)
sv[initIdx[i]]=0;

#ifdef _OPENMP
size_t sidxSize=sidx.size();
#pragma omp parallel for if(sidxSize>20)
for (size_t i=0; i<sidxSize; ++i)
atomic_ref(sv[sidx[i]]) += fv[fidx[i]] * m[i];
Comment on lines +102 to +105

@coderabbitai coderabbitai Bot Oct 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix the atomic_ref instantiation

atomic_ref is a class template; atomic_ref(...) += ...; is ill-formed, so the OpenMP build will not compile. Please instantiate a std::atomic_ref<double> (and include <atomic> with these OpenMP additions).

-#pragma omp parallel for if(initIdxSize>20)
-    for (size_t i=0; i<sidxSize; ++i)
-      atomic_ref(sv[sidx[i]]) += fv[fidx[i]] * m[i];
+#pragma omp parallel for if(initIdxSize>20)
+    for (size_t i=0; i<sidxSize; ++i)
+      {
+        std::atomic_ref<double> cell{sv[sidx[i]]};
+        cell += fv[fidx[i]] * m[i];
+      }

Remember to add #include <atomic> with the other includes.

🤖 Prompt for AI Agents 
In engine/evalGodley.cc around lines 102 to 105, replace the ill-formed use of
atomic_ref(...) += ... with a properly instantiated std::atomic_ref<double>; add
#include <atomic> to the includes. Inside the OpenMP loop create a local
std::atomic_ref<double> (e.g. std::atomic_ref<double> ar(sv[sidx[i]]);) and
perform the addition via that atomic_ref (ar += fv[fidx[i]] * m[i]; or
ar.fetch_add(...)); this fixes the template instantiation and ensures
thread-safe atomic updates.

@highperformancecoder highperformancecoder Oct 5, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

It does compile - it makes use of a new C++17 feature called class template argument deduction

@coderabbitai coderabbitai Bot Oct 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@highperformancecoder, you're absolutely right! I apologize for the incorrect suggestion. The atomic_ref(sv[sidx[i]]) syntax is indeed valid with class template argument deduction (CTAD), which allows the compiler to deduce the template parameter <double> from the constructor argument.

Just a minor clarification: std::atomic_ref was introduced in C++20 rather than C++17, though CTAD itself is a C++17 feature.

Since you've confirmed it compiles successfully, the code is fine as-is. Just ensure that <atomic> is included (either directly or transitively through one of the headers) for atomic_ref to be available.

Thank you for the correction!

#else
for (size_t i=0; i<sidx.size(); ++i)
sv[sidx[i]] += fv[fidx[i]] * m[i];
#endif
Comment on lines +101 to +109

Copilot AI Oct 5, 2025

Copy link

Choose a reason for hiding this comment

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

Two issues: (1) The if-clause on the pragma uses initIdxSize instead of sidxSize, so the parallelization condition is incorrect. (2) atomic_ref is used as if it were a function; it should be std::atomic_ref with fetch_add (and requires ). Suggested fix: use if(sidxSize>20) and replace the body with std::atomic_ref ref(sv[sidx[i]]); ref.fetch_add(fv[fidx[i]] * m[i], std::memory_order_relaxed);

Copilot uses AI. Check for mistakes.

@highperformancecoder highperformancecoder Oct 5, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

atomic_ref "used as afunction" is actually instantiating an unnamed temporary variable, and the += operator is overloaded to call fetch_add. So equivalent code, and idiomatic C++.

@highperformancecoder highperformancecoder Oct 5, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

I fixed the incorrect #pragma - thanks.

}
}
CLASSDESC_ACCESS_EXPLICIT_INSTANTIATION(minsky::EvalGodley);
75 changes: 59 additions & 16 deletions engine/minskyTensorOps.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,19 +347,34 @@ namespace minsky
template <>
struct GeneralTensorOp<OperationType::all>: public civita::ReductionOp
{
GeneralTensorOp(): civita::ReductionOp([](double& x, double y,size_t){x*=(y>0.5);},1){}
GeneralTensorOp(): civita::ReductionOp([](double& x, double y,size_t){
#ifdef _OPENMP
#pragma omp atomic
#endif
x*=(y>0.5);
},1){}
};

template <>
struct GeneralTensorOp<OperationType::runningSum>: public civita::Scan
{
GeneralTensorOp(): civita::Scan([](double& x,double y,size_t){x+=y;}) {}
GeneralTensorOp(): civita::Scan([](double& x,double y,size_t){
#ifdef _OPENMP
#pragma omp atomic
#endif
x+=y;
}) {}
};

template <>
struct GeneralTensorOp<OperationType::runningProduct>: public civita::Scan
{
GeneralTensorOp(): civita::Scan([](double& x,double y,size_t){x*=y;}) {}
GeneralTensorOp(): civita::Scan([](double& x,double y,size_t){
#ifdef _OPENMP
#pragma omp atomic
#endif
x*=y;
}) {}
};

template <>
Expand Down Expand Up @@ -1518,16 +1533,28 @@ namespace minsky

double operator[](size_t i) const override {
double v=0;
if (!chain.empty())
bool redoCalc=false;
do
{
v=(*chain.back())[i];
if (m_timestamp<chain.back()->timestamp())
{ // update hypercube if argument has changed
const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube());
chain=ravel::createRavelChain(ravel.getState(), arg);
m_timestamp=Timestamp::clock::now();
if (!chain.empty())
{
v=(*chain.back())[i];
#ifdef _OPENMP
#pragma omp barrier
#pragma omp single copyprivate(redoCalc)
#endif
{
redoCalc=m_timestamp<chain.back()->timestamp();
if (redoCalc)
{ // update hypercube if argument has changed
const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube());
chain=ravel::createRavelChain(ravel.getState(), arg);
m_timestamp=Timestamp::clock::now();
}
}
}
}
while (redoCalc);
return v;
}
size_t size() const override {return chain.empty()? 1: chain.back()->size();}
Expand Down Expand Up @@ -1683,12 +1710,28 @@ namespace minsky
throw FlowVarsResized();
result.ev->update(fv, n, sv);
assert(result.size()==rhs->size());
for (size_t i=0; i<rhs->size(); ++i)
{
auto v=(*rhs)[i];
result[i]=v;
assert(!isfinite(result[i]) || fv[result.idx()+i]==v);
}
// the assumption here is that rhs is independent of rhs
auto rhsSize=rhs->size();
string errMsg;
#ifdef _OPENMP
#pragma omp parallel for if(rhsSize>20)
#endif
for (size_t i=0; i<rhsSize; ++i)
try
{
auto v=(*rhs)[i];
result[i]=v;
assert(!isfinite(result[i]) || fv[result.idx()+i]==v);
}
catch (const std::exception& ex)
{
#ifdef _OPENMP
#pragma omp critical
#endif
errMsg=ex.what();
}
if (!errMsg.empty())
throw runtime_error(errMsg);
}
}

Expand Down
31 changes: 30 additions & 1 deletion engine/rungeKutta.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,36 @@ namespace minsky
// firstly evaluate the flow variables. Initialise to flowVars so
// that no input vars are correctly initialised
auto flow(flowVars);

for (size_t i=0; i<equations.size(); ++i)
equations[i]->eval(flow.data(), flow.size(), vars);
// TODO - equations are ordered, so we can't naively parallelise this loop.
// auto eqSize=equations.size();
// string threadErrMsg;
//#ifdef _OPENMP
// //#pragma omp parallel for if(eqSize>20)
//#endif
// for (size_t i=0; i<eqSize; ++i)
// try
// {
// equations[i]->eval(flow.data(), flow.size(), vars);
// }
// catch (const std::exception& e)
// {
//#ifdef _OPENMP
//#pragma omp critical
//#endif
// threadErrMsg=e.what();
// }
// if (!threadErrMsg.empty())
// throw runtime_error(threadErrMsg);

// then create the result using the Godley table
for (size_t i=0; i<stockVars.size(); ++i) result[i]=0;
auto stvSize=stockVars.size();
#ifdef _OPENMP
#pragma omp parallel for if(stvSize>20)
#endif
for (size_t i=0; i<stvSize; ++i) result[i]=0;
evalGodley.eval(result, flow.data());

// integrations are kind of a copy
Expand All @@ -270,6 +295,10 @@ namespace minsky
}
// enable element-wise integration of tensor variables. for feature 147
assert(i->input().size()==i->stock->size());
auto inputSize=i->input().size();
#ifdef _OPENMP
#pragma omp parallel for if(inputSize>20)
#endif
for (size_t j=0; j<i->input().size(); ++j)
result[i->stock->idx()+j] = reverseFactor *
(i->input().isFlowVar()? flow[i->input().idx()+j] : vars[i->input().idx()+j]);
Expand Down
7 changes: 4 additions & 3 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ else
LIBS+=-lxcb
endif

ifdef OPENMP
FLAGS+=-fopenmp

Copilot AI Oct 5, 2025

Copy link

Choose a reason for hiding this comment

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

For some toolchains, linking requires -fopenmp as well. Consider also adding LIBS+=-fopenmp (or LDFLAGS) under the same OPENMP guard to avoid link errors.

Suggested change
FLAGS+=-fopenmp
FLAGS+=-fopenmp
LIBS+=-fopenmp

Copilot uses AI. Check for mistakes.

@highperformancecoder highperformancecoder Oct 5, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

OK

LIBS+=-fopenmp
endif
Comment on lines +29 to +32

@coderabbitai coderabbitai Bot Oct 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Don’t hard‑code -fopenmp; respect the caller’s flag.

ifdef OPENMP now unconditionally adds -fopenmp to both FLAGS and LIBS. That breaks toolchains that need a different switch (e.g., -qopenmp, -mp, clang’s -Xpreprocessor -fopenmp + -lomp) because they’ll still inherit this hard-coded GNU flag and fail to compile or link. Please append the user-provided $(OPENMP) value (and, if needed, a separate $(OPENMP_LIBS)) instead of forcing -fopenmp, so downstream make invocations can supply the appropriate options.

🤖 Prompt for AI Agents 
In test/Makefile around lines 29 to 32, the Makefile currently hard-codes
-fopenmp into FLAGS and LIBS when OPENMP is defined; instead, respect
caller-provided flags by appending the variable values. Change the logic so that
if OPENMP is set you append $(OPENMP) to FLAGS (and optionally append
$(OPENMP_LIBS) to LIBS) rather than writing -fopenmp; ensure existing behavior
remains when callers export OPENMP or OPENMP_LIBS (so downstream toolchains can
pass their preferred compile/link switches).

@highperformancecoder highperformancecoder Oct 5, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

That's not how I intend to use it. The OPENMP flag is defined or not to enable OpenMP or not. We're currently supporting gcc and clang (or dpcpp, which is based on clang) only - gcc ignores -fopenmp on linking and clang requires it.

@coderabbitai coderabbitai Bot Oct 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@highperformancecoder, understood! Given that you're supporting gcc and clang/dpcpp exclusively, and -fopenmp works correctly for both (gcc ignoring it on link, clang requiring it), your boolean switch approach is perfectly reasonable. Thank you for clarifying the design intent.


# RSVG dependencies calculated here
FLAGS+=$(shell pkg-config --cflags librsvg-2.0)
Expand All @@ -34,9 +38,6 @@ LIBS+=$(shell pkg-config --libs librsvg-2.0)
EXES=cmpFp checkSchemasAreSame
#testDatabase testGroup

ifdef AEGIS
#EXES+=tcl-cov
endif

ifdef ASAN
FLAGS+=-fsanitize=address -fno-omit-frame-pointer
Expand Down