Repository navigation
OpenMP support of tensor processing. - #565
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds conditional OpenMP support in build scripts and parallelizes selected compute paths in tensor ops, Runge–Kutta evaluation, and Godley evaluation. Introduces atomic/atomic_ref guards, conditional thresholds (>20) for parallel loops, error aggregation in parallel blocks, and a redoCalc flag enabling conditional RavelTensor recomputation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Th as Threads (_OPENMP)
participant TE as TensorEval::eval
participant RHS as rhs tensor
participant RES as result tensor
note over TE,RHS: Conditional parallel copy if rhsSize > 20
alt _OPENMP and rhsSize > 20
Th->>RES: for i in [0..n): RES[i] = RHS[i]
Note over Th,RES: Capture first thrown error into shared threadErrMsg
Th-->>TE: if threadErrMsg set -> throw runtime_error
else
TE->>RES: Sequential copy loop
end
sequenceDiagram
autonumber
participant RT as RavelTensor
participant Arg as Underlying arg
note over RT,Arg: Conditional recomputation when Arg timestamp newer
alt redoCalc true (single thread section)
RT->>RT: Recompute hypercubes and chain
RT->>RT: Reset redoCalc=false
else
RT->>RT: Use existing chain
end
sequenceDiagram
autonumber
actor Th as Threads (_OPENMP)
participant EG as EvalGodley::eval
participant SV as sv vector
participant IDX as initIdx / sidx
Note over EG,SV: Initialize SV[initIdx]=0 (size threshold)
alt _OPENMP and |initIdx| > 20
Th->>SV: parallel for i in initIdx: SV[i]=0
else
EG->>SV: sequential init
end
Note over EG,SV: Accumulate over sidx into SV with atomic
alt _OPENMP and |sidx| > 20
Th->>SV: parallel for k in sidx: atomic_add(SV[...], ...)
else
EG->>SV: sequential accumulation
end
sequenceDiagram
autonumber
actor Th as Threads (_OPENMP)
participant RK as RungeKutta::evalEquations
participant ST as stock vars
participant TNS as tensor inputs
Note over RK,ST: Initialize stocks (thresholded)
alt _OPENMP and |ST| > 20
Th->>ST: parallel init
else
RK->>ST: sequential init
end
Note over RK,TNS: Element-wise tensor integration (thresholded)
alt _OPENMP and |TNS| > 20
Th->>TNS: parallel integrate elements
else
RK->>TNS: sequential integration
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Makefile(2 hunks)RavelCAPI(1 hunks)engine/minskyTensorOps.cc(2 hunks)engine/rungeKutta.cc(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
engine/minskyTensorOps.cc (1)
engine/variableValue.cc (2)
i(95-99)i(95-95)
engine/rungeKutta.cc (2)
model/group.cc (1)
flow(58-58)schema/schema3.cc (1)
flow(866-866)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (2)
Makefile (1)
31-31: OpenMP flag never reaches the compile commands.Line 31 now forwards
OPENMP=$(OPENMP)to sub-makes, but this Makefile itself never consumes the variable. If I build withmake OPENMP=-fopenmp, the compilation lines still omit-fopenmp, so_OPENMPremains undefined and the new parallel code paths stay disabled (only the link step sees-fopenmpvia the clang-specific block on Line 216). Please append$(OPENMP)to the compile and link flags—e.g. add anifdef OPENMPsection that doesFLAGS += $(OPENMP)and reuses the same variable forLIBS—so the setting actually enables OpenMP in this TU. request_verificationengine/rungeKutta.cc (1)
256-260: OpenMP parallel loop breaks exception propagation.Line 258 wraps
equations[i]->eval(...)in an OpenMP parallel region. Upstream (RungeKutta::RKfunction, Lines 50‑63) explicitly relies onevalEquationsthrowing so it can surface solver errors viathreadErrMsg. Under OpenMP, any exception escaping a parallel region triggersstd::terminate, so scenarios that used to report “Invalid arithmetic operation detected”, “integral not wired”, etc., will now hard-abort the process. Please keep this loop sequential or add per-iteration try/catch that records the first failure and aborts the parallel work without letting the exception cross the region (e.g. with an atomic flag + early exit). Until that’s in place this change is unsafe. raise_critical_issue
| auto chainBack=chain.back(); // stash a reference to the current chain, in case it gets updated below | ||
| v=(*chainBack)[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(); | ||
| } | ||
| #ifdef _OPENMP | ||
| #pragma omp critical(RavelTensor_updateChain) | ||
| if (m_timestamp<chain.back()->timestamp()) // test again once lock has been obtained | ||
| #endif | ||
| { // update hypercube if argument has changed | ||
| const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube()); | ||
| chain=ravel::createRavelChain(ravel.getState(), arg); | ||
| m_timestamp=Timestamp::clock::now(); | ||
| } |
There was a problem hiding this comment.
Synchronize all reads of chain/m_timestamp
We still read chain.back() and m_timestamp outside the RavelTensor_updateChain critical section while the write happens inside it. That’s a data race: another thread can be in the critical block assigning a new chain vector (or updating m_timestamp) while this thread reads, yielding undefined behaviour. Pull the chain access and timestamp check inside the same critical section (or protect them with atomics) so every read and write is synchronized.
Apply this diff to guard all accesses consistently:
- {
- auto chainBack=chain.back(); // stash a reference to the current chain, in case it gets updated below
- v=(*chainBack)[i];
- if (m_timestamp<chain.back()->timestamp())
-#ifdef _OPENMP
-#pragma omp critical(RavelTensor_updateChain)
- if (m_timestamp<chain.back()->timestamp()) // test again once lock has been obtained
-#endif
- { // update hypercube if argument has changed
- const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube());
- chain=ravel::createRavelChain(ravel.getState(), arg);
- m_timestamp=Timestamp::clock::now();
- }
- }
+ {
+ TensorPtr chainBack;
+#ifdef _OPENMP
+#pragma omp critical(RavelTensor_updateChain)
+#endif
+ {
+ if (m_timestamp<chain.back()->timestamp())
+ {
+ const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube());
+ chain=ravel::createRavelChain(ravel.getState(), arg);
+ m_timestamp=chain.back()->timestamp();
+ }
+ chainBack=chain.back();
+ }
+ v=(*chainBack)[i];
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| auto chainBack=chain.back(); // stash a reference to the current chain, in case it gets updated below | |
| v=(*chainBack)[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(); | |
| } | |
| #ifdef _OPENMP | |
| #pragma omp critical(RavelTensor_updateChain) | |
| if (m_timestamp<chain.back()->timestamp()) // test again once lock has been obtained | |
| #endif | |
| { // update hypercube if argument has changed | |
| const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube()); | |
| chain=ravel::createRavelChain(ravel.getState(), arg); | |
| m_timestamp=Timestamp::clock::now(); | |
| } | |
| { | |
| TensorPtr chainBack; | |
| #ifdef _OPENMP | |
| #pragma omp critical(RavelTensor_updateChain) | |
| #endif | |
| { | |
| // all shared reads/writes now inside the critical section | |
| if (m_timestamp < chain.back()->timestamp()) | |
| { | |
| const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube()); | |
| chain = ravel::createRavelChain(ravel.getState(), arg); | |
| m_timestamp = chain.back()->timestamp(); | |
| } | |
| chainBack = chain.back(); | |
| } | |
| v = (*chainBack)[i]; | |
| } |
🤖 Prompt for AI Agents
In engine/minskyTensorOps.cc around lines 1523 to 1534, reads of chain and
m_timestamp occur outside the RavelTensor_updateChain critical section while
writes occur inside, causing a data race; move the reads (chain.back(), v
assignment and the m_timestamp check) into the same omp
critical(RavelTensor_updateChain) block (or make chain/m_timestamp
atomic/lock-protected) so that the timestamp comparison and any use of chain
happen only under the same lock used for updates, then re-check condition inside
the lock and perform populateHypercube/createRavelChain/update of m_timestamp
while holding the lock.
There was a problem hiding this comment.
Your suggestion serialises the loop. As additional context, chain is a vector of shared_ptrs. Reading chain.back() into chainBack is atomic, and all that happens in that potentially the value of an old chain is returned, rather than the updated chain.
There was a problem hiding this comment.
@highperformancecoder, you're absolutely right that the suggested fix would serialize all calls to operator[], which defeats the purpose of OpenMP parallelization. My apologies for that oversight.
However, there is still a subtle race condition. While copying a shared_ptr is thread-safe, reading from a std::vector while another thread reassigns that vector is undefined behavior according to the C++ standard. The line chain=ravel::createRavelChain(...) inside the critical section replaces the entire vector, which can corrupt reads from threads outside the critical section calling chain.back().
Better alternatives that preserve parallelism:
- Accept stale reads with proper barriers (if old chain values are acceptable):
auto chainBack = chain.back(); // relaxed read
v = (*chainBack)[i];
// Then do the timestamp check & update as you have itDocument that concurrent reads may use stale chain data, which seems acceptable based on your comment.
-
Use
std::atomic<Timestamp>for the timestamp and keep chain updates inside the critical section as-is. This ensures the timestamp comparison is race-free without serializing the data access. -
Reader-writer lock (e.g.,
std::shared_mutex) to allow concurrent reads while serializing updates only.
Would option 1 (accepting occasional stale reads with the current update mechanism) work for your use case? If the old chain returning correct-enough results is acceptable, you might just need to add a comment documenting this behavior.
There was a problem hiding this comment.
Hmm - maybe a reader-writer lock would be a better approach on dereferencing chain.back().
The problem is the calculation should be redone if ever the chain is rebuilt. In the sequential case, the chain is only ever rebuilt if the first iteration causes a timestamp update.
Possibly I need to do a barrier, and check for chain update, and redo the calculation if there's an update...
There was a problem hiding this comment.
@highperformancecoder, you've identified the core issue precisely. Let me outline the three main approaches:
1. Reader-Writer Lock (Recommended)
This is the cleanest solution for preserving correctness while allowing parallel reads:
double operator[](size_t i) const override {
double v=0;
if (!chain.empty())
{
std::shared_lock<std::shared_mutex> readLock(chainMutex);
auto chainBack=chain.back();
auto needsUpdate = m_timestamp < chainBack->timestamp();
readLock.unlock();
if (needsUpdate)
{
std::unique_lock<std::shared_mutex> writeLock(chainMutex);
// Double-check after acquiring exclusive lock
if (m_timestamp < chain.back()->timestamp())
{
const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube());
chain=ravel::createRavelChain(ravel.getState(), arg);
m_timestamp=Timestamp::clock::now();
}
chainBack=chain.back();
}
v=(*chainBack)[i];
}
return v;
}Pros: Clean semantics, all threads see consistent chain state
Cons: Requires C++17's std::shared_mutex
2. Barrier + Recompute Pattern
Check if chain changed after parallel region completes, redo if necessary:
In TensorEval::eval, wrap the parallel loop:
bool chainChanged;
do {
chainChanged = false;
auto initialTimestamp = rhs_as_ravel_tensor->captureTimestamp();
#pragma omp parallel for
for (size_t i=0; i<rhsSize; ++i) {
auto v=(*rhs)[i];
result[i]=v;
}
if (rhs_as_ravel_tensor->timestampChanged(initialTimestamp))
chainChanged = true;
} while(chainChanged);Pros: Ensures all results come from same chain version
Cons: Complex, potential for wasted work if chain updates frequently
3. Single-Threaded Pre-Check (Simplest)
Before the parallel loop in TensorEval::eval, force chain update in a single-threaded context:
// Ensure chain is current before parallel execution
if (auto rt = dynamic_cast<RavelTensor*>(rhs.get()))
rt->ensureChainCurrent();
#pragma omp parallel for if(rhsSize>20)
for (size_t i=0; i<rhsSize; ++i) {
...
}Pros: Simple, no mid-iteration updates
Cons: Small serialization point; chain could still update during iteration if timestamps drift
Given your performance requirements and the codebase patterns, I'd recommend Option 1 (reader-writer lock) as it provides clear correctness guarantees while keeping reads concurrent. The overhead of shared_lock is minimal when no writes occur.
Thanks - that was left in from an earlier experiment. The dependent libs no longer depend on OpenMP, so that flag can be removed.
This is very important. Thanks for pointing that out. |
There was a problem hiding this comment.
Pull Request Overview
Adds conditional OpenMP-based parallelism to heavy evaluation paths (tensor ops, Godley table accumulation, and integration loops) and propagates OpenMP flags in the build to improve multi-core performance on large models.
- Introduces OpenMP pragmas for selected loops with size-based thresholds.
- Updates build/test Makefiles to add OpenMP flags; bumps RavelCAPI submodule.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| engine/rungeKutta.cc | Adds optional OpenMP loop parallelism and error aggregation for equation evaluation, zeroing, and element-wise integration. |
| engine/minskyTensorOps.cc | Adds OpenMP atomics in tensor reductions/scans; modifies Ravel::operator[] logic for recalculation. |
| engine/evalGodley.cc | Parallelizes initialization and accumulation; attempts to use atomic_ref for concurrent accumulation. |
| test/Makefile | Adds OPENMP gating for -fopenmp in FLAGS for tests. |
| Makefile | Adds -fopenmp to LIBS when using clang++. |
| RavelCAPI | Updates submodule pointer. |
Comments suppressed due to low confidence (1)
engine/rungeKutta.cc:1
- The comment likely intended 'rhs is independent of result' (or of the loop index). Please clarify to avoid confusion.
/*
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #ifdef _OPENMP | ||
| size_t sidxSize=sidx.size(); | ||
| #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]; | ||
| #else | ||
| for (size_t i=0; i<sidx.size(); ++i) | ||
| sv[sidx[i]] += fv[fidx[i]] * m[i]; | ||
| #endif |
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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++.
There was a problem hiding this comment.
I fixed the incorrect #pragma - thanks.
| 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]; | ||
|
|
There was a problem hiding this comment.
chain.back() is accessed unconditionally at line 1543, which is undefined behavior when chain is empty. Additionally, v is computed twice when chain is non-empty. Guard the early return when chain is empty and remove the duplicate evaluation, e.g., if (chain.empty()) return 0; auto chainBack = chain.back(); v = (*chainBack)[i];
There was a problem hiding this comment.
Your're right - there does need to be a barrier before the single region, as well as after.
| #ifdef _OPENMP | ||
| #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()); |
There was a problem hiding this comment.
omp single is only meaningful inside a parallel region; here it is outside any #pragma omp parallel and will be ignored or warn. If the intent is to synchronize an update across threads, either protect the mutation with a mutex/critical section or place this block inside an explicit parallel region with single (or critical) and adjust variable scoping accordingly.
There was a problem hiding this comment.
It is called from within a parallel region mostly. If not, then I would expect the pragmas to be nops.
| mutable vector<TensorPtr> chain; | ||
| TensorPtr arg; | ||
| mutable Timestamp m_timestamp; | ||
| bool redoCalc=false; |
There was a problem hiding this comment.
Member redoCalc is unused and shadowed by the local redoCalc in operator[]. Remove the member or use it consistently to avoid confusion.
| bool redoCalc=false; |
There was a problem hiding this comment.
Thanks for picking that up.
| { | ||
| equations[i]->eval(flow.data(), flow.size(), vars); | ||
| } | ||
| catch (std::exception& e) |
There was a problem hiding this comment.
Catch exceptions by const reference: catch (const std::exception& e) to avoid unintended modifications and allow binding to temporaries.
| catch (std::exception& e) | |
| catch (const std::exception& e) |
| endif | ||
|
|
||
| ifdef OPENMP | ||
| FLAGS+=-fopenmp |
There was a problem hiding this comment.
For some toolchains, linking requires -fopenmp as well. Consider also adding LIBS+=-fopenmp (or LDFLAGS) under the same OPENMP guard to avoid link errors.
| FLAGS+=-fopenmp | |
| FLAGS+=-fopenmp | |
| LIBS+=-fopenmp |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
engine/minskyTensorOps.cc (1)
1536-1557: Chain updates still race with parallel readersThe new
omp singleblock updateschainwhile the surrounding code keeps callingchain.back()outside any synchronization. One thread can still reassignchaininside the single region while another thread is executingchain.back()(orchain.empty()) in this same method, which is undefined behaviour onstd::vector. This is the same race called out in the earlier review—writes are protected but reads are not.Please guard both the read and the possible rebuild with the same critical/lock so every access is synchronized. For example:
- bool redoCalc=false; - do - { - if (!chain.empty()) - v=(*chain.back())[i]; - - auto chainBack=chain.back(); // stash a reference to the current chain, in case it gets updated below - v=(*chainBack)[i]; -#ifdef _OPENMP -#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); + bool redoCalc=false; + do + { +#ifdef _OPENMP +#pragma omp critical(RavelTensor_updateChain) +#endif + { + if (chain.empty()) + return v; + auto chainBack=chain.back(); + redoCalc=m_timestamp<chainBack->timestamp(); + if (redoCalc) + { + const_cast<Ravel&>(ravel).populateHypercube(arg->hypercube()); + chain=ravel::createRavelChain(ravel.getState(), arg); + m_timestamp=Timestamp::clock::now(); + chainBack=chain.back(); + } + v=(*chainBack)[i]; + } + } + while (redoCalc);Any equivalent scheme that synchronizes every read with the write (e.g.,
std::shared_mutex) would work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)engine/evalGodley.cc(1 hunks)engine/minskyTensorOps.cc(4 hunks)engine/rungeKutta.cc(2 hunks)test/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
| size_t sidxSize=sidx.size(); | ||
| #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]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It does compile - it makes use of a new C++17 feature called class template argument deduction
There was a problem hiding this comment.
@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!
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…er/minsky into feature-enable-openmp
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)engine/evalGodley.cc(1 hunks)engine/minskyTensorOps.cc(3 hunks)engine/rungeKutta.cc(2 hunks)test/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧬 Code graph analysis (1)
engine/minskyTensorOps.cc (2)
model/operation.cc (4)
x(48-48)x(48-48)x(54-54)x(54-54)engine/variableValue.cc (2)
i(95-99)i(95-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
- GitHub Check: Analyze (javascript, 22.04)
- GitHub Check: ubuntu-20.04
- GitHub Check: ubuntu-20.04
🔇 Additional comments (6)
engine/minskyTensorOps.cc (5)
350-355: LGTM: Correct atomic operation for thread safety.The
#pragma omp atomiccorrectly protects thex*=(y>0.5)operation from data races when multiple threads update the same accumulator in the reduction operation.
361-366: LGTM: Correct atomic operation for thread safety.The
#pragma omp atomiccorrectly protects thex+=yoperation from data races during the running sum computation.
372-377: LGTM: Correct atomic operation for thread safety.The
#pragma omp atomiccorrectly protects thex*=yoperation from data races during the running product computation.
1713-1734: LGTM: Correct exception handling pattern for OpenMP.The parallel loop properly captures exceptions into
errMsgusing a critical section (lines 1728-1731) and rethrows after the parallel region completes (lines 1733-1734). This prevents exceptions from escaping the parallel region, which would otherwise causestd::terminate.
1536-1557: OpenMP pragmas execute only within the#pragma omp parallel forinTensorEval::eval; no additional synchronization is required.engine/rungeKutta.cc (1)
298-305: Remove exception-wrapping suggestion for the OpenMP loop The loop body uses onlyoperator[]and basic arithmetic (all effectively noexcept), so a try-catch is unnecessary.Likely an incorrect or invalid review comment.
| ifdef OPENMP | ||
| FLAGS+=-fopenmp | ||
| LIBS+=-fopenmp | ||
| endif |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
This change is
Summary by CodeRabbit
New Features
Chores
Tests