Feature/add test script#1
Open
gihanmudalige wants to merge 9 commits into
Open
Conversation
…de bug The OP2 CUDA code generator applies a SoA dat stride to all pointer accesses inside vec_norm3. When thin_layer_viscous_flux_op2 called vec_norm3(delta_r) with a local 3-element array, it expanded to delta_r[k * round32(n_edges)], causing an illegal memory access. Only flatplate was affected because it is the only config with include_viscous=1. Fix: replace vec_norm3(delta_r) with an inline sqrt expression so the generator cannot apply a dat stride to the local array indices. NOTE: the matching fix in the generated GPU files under generated/ (gitignored) requires deleting the stale op2_kernels.o and rebuilding: rm generated/nssolver_op2/cuda/op2_kernels.o && make nssolver_op2_cuda
There was a problem hiding this comment.
Pull request overview
This PR is titled "add test script" but actually bundles four unrelated changes: a new bash test harness that builds and exercises all nssolver_op2 binary variants against the public mesh configs, a portability refactor of op2_kernels.h that replaces std:: math/clamp/etc. with C <math.h> equivalents and constexpr constants with #define macros, a Makefile rpath addition for HDF5 helper binaries, and a reorder of op_partition before op_decl_dat_temp_char calls in nssolver_op2.cpp.
Changes:
- New
tests/nssolver_op2_test.shtest harness driving build + run phases across seq/genseq/openmp/cuda/c_cuda and their MPI equivalents. op2_kernels.hconverted from C++ std lib (std::max/min/abs/clamp/isfinite/sqrt/pow) to C<math.h>functions, andconstexprconstants replaced with#defines — thoughconstexpr, lambdas, andstatic_castremain in the file.- Makefile gains
-Wl,-rpath,$(HDF5_HELPER_LIBDIR);nssolver_op2.cppmoves theop_partitioncall ahead of the tempop_datdeclarations.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| tests/nssolver_op2_test.sh | New harness; has set -e/grep interaction bug, skip-vs-fail inconsistency, relative-path issue before cd, and possible mesh-name mismatch with flatplate_develop.cfg. |
| op2_kernels.h | Std-lib→C math substitutions; partial C-compat claim (constexpr/lambda/static_cast remain), clamp→fmax/fmin differs on NaN, one site replaces vec_norm3 with an inline sqrt inconsistently. |
| nssolver_op2.cpp | Moves op_partition("PARMETIS","KWAY",...) earlier — non-trivial reorder unrelated to the PR title. |
| Makefile | Adds rpath flag to HELPER_LDLIBS; second hunk is a no-op whitespace/context line. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (fabs(projected_delta) < 1.0e-14) return 1.0; | ||
| if (projected_delta * edge_delta <= 0.0) return 0.0; | ||
| return std::clamp(edge_delta / projected_delta, 0.0, 1.0); | ||
| return fmax(0.0, fmin(1.0, edge_delta / projected_delta)); |
| inline void thin_layer_viscous_flux_op2(const double *pl, const double *pr, const double *delta_r, const double *area_vector, | ||
| double *flux) { | ||
| const double distance = std::max(vec_norm3(delta_r), 1.0e-14); | ||
| const double distance = fmax(sqrt(delta_r[0] * delta_r[0] + delta_r[1] * delta_r[1] + delta_r[2] * delta_r[2]), 1.0e-14); |
Comment on lines
+45
to
+67
| function is_file_available { | ||
| local file="$1" | ||
| if [ -f "$file" ]; then | ||
| echo "$file exists" >> "$LOG" | ||
| return 0 | ||
| else | ||
| echo "TEST FAILED : $file is missing" >> "$LOG" | ||
| return 1 | ||
| fi | ||
| } | ||
|
|
||
| # validate "<prepend>" "<binary>" "<args>" "<grep_word>" | ||
| function validate { | ||
| local prepend="$1" | ||
| local bin="$2" | ||
| local args="$3" | ||
| local grep_word="$4" | ||
|
|
||
| if ! is_file_available "$bin"; then | ||
| echo "SKIPPING: $bin does not exist" | tee -a "$LOG" | ||
| echo "" | tee -a "$LOG" | ||
| return | ||
| fi |
| echo "Preprocessing meshes..." | tee -a "$LOG" | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" box meshes-op2/box.h5 | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" bump meshes-op2/bump.h5 | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" flatplate meshes-op2/flatplate.h5 |
Comment on lines
+134
to
+141
| echo "Preprocessing meshes..." | tee -a "$LOG" | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" box meshes-op2/box.h5 | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" bump meshes-op2/bump.h5 | ||
| "$APP_DIR/scripts/preprocess_mesh.sh" flatplate meshes-op2/flatplate.h5 | ||
|
|
||
| cd "$APP_DIR" | ||
| echo "Building all app variants..." | tee -a "$LOG" | ||
| make all |
Comment on lines
+82
to
+85
| set +e | ||
| eval "$cmd" > "$SCRIPT_RUN_LOC/perf_out_$file_tail" 2>&1 | ||
| local run_rc=$? | ||
| set -e |
| RUN_TESTS=${RUN_TESTS:-FALSE} | ||
|
|
||
| # MPI process counts (tune for available hardware) | ||
| MPI_NP_SEQ=${MPI_NP_SEQ:-4} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request primarily refactors the
op2_kernels.hfile to remove C++ standard library dependencies and replace them with C-style math functions and macros, improving portability and compatibility with C-based toolchains. Additionally, it makes a minor linker flag improvement in theMakefileand reorders a function call innssolver_op2.cpp. The most important changes are grouped below:Refactoring to Remove C++ Standard Library Dependencies
Replaced all uses of C++ standard library functions (e.g.,
std::max,std::min,std::abs,std::clamp,std::isfinite,std::sqrt,std::pow) with their C equivalents (e.g.,fmax,fmin,fabs,isfinite,sqrt,pow) throughoutop2_kernels.h. This includes updating all relevant mathematical and utility operations in kernel functions.Replaced
constexprconstants with#definemacros and removed C++ headers (<algorithm>,<cmath>), replacing them with the C header<math.h>.Build System Improvement
-Wl,-rpath,$(HDF5_HELPER_LIBDIR)to theHELPER_LDLIBSvariable in theMakefileto help the linker find HDF5 libraries at runtime.Minor Logic/Ordering Change
op_partition("PARMETIS", "KWAY", ...)earlier innssolver_op2.cppto occur before the declaration of temporaryop_datvariables, possibly to ensure correct partitioning before data allocation.