Repository navigation
Matlab mex cpp interface - #15
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Introduces an initial MATLAB-native C++ MEX “optimized runtime” (with build/test/package tooling) and wires it into repo setup/benchmark documentation, while adding planning docs for a separate Python-only optimized path.
Changes:
- Add a new
matlab/cpp_mex/C++ MEX implementation (probe entrypoints +info_cc_slice_cpp/info_cd_slice_cpp) with MATLABbuildtooltasks and smoke tests. - Teach
setup_gcmi(and docs) to auto-addmatlab/cpp_mex/bin/<release>/<mexext>/when present and update benchmark harness to support multiple optimized backends + label encodings. - Add Python optimization spec/backlog docs and remove older MATLAB build-cleanup plan documents.
Reviewed changes
Copilot reviewed 28 out of 31 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| setup_gcmi.m | Adds IncludeNative option and auto-path discovery for compiled C++ MEX binaries. |
| README.md | Documents auto-runtime discovery and how to build/smoke-test the new C++ MEX path. |
| plans/PYTHON_OPTIMIZED_IMPLEMENTATION_SPEC.md | New design spec for a Python-only optimized path (Numba-first). |
| plans/PYTHON_OPTIMIZED_IMPLEMENTATION_BACKLOG.md | New execution backlog/tasks for the Python optimization effort. |
| plans/MATLAB_BUILD_CLEANUP_SPEC.md | Removes prior MATLAB build-cleanup spec document. |
| plans/MATLAB_BUILD_CLEANUP_BACKLOG.md | Removes prior MATLAB build-cleanup backlog document. |
| matlab/cpp_mex/tooling/gcmi_cpp_mex_test.m | Adds compile-and-smoke-test runner for the new C++ MEX entrypoints. |
| matlab/cpp_mex/tooling/gcmi_cpp_mex_package.m | Adds packaging helper to zip compiled artifacts per release/arch. |
| matlab/cpp_mex/tooling/gcmi_cpp_mex_config.m | Adds compiler/OpenMP configuration discovery for building C++ MEX targets. |
| matlab/cpp_mex/tooling/gcmi_cpp_mex_compile.m | Adds multi-target MEX build driver + build metadata emission. |
| matlab/cpp_mex/tooling/gcmi_cpp_mex_clean.m | Adds cleanup helper for build/output directories. |
| matlab/cpp_mex/tests/run_gcmi_cpp_mex_smoke_tests.m | Adds a minimal test entrypoint for matlab/cpp_mex smoke tests. |
| matlab/cpp_mex/src/info_cc_slice_bridge.cpp | Adds classic-C API bridge for a C-API MEX wrapper target. |
| matlab/cpp_mex/src/gcmi_kernels.cpp | Implements the core C++ kernels (info_cc_slice, info_cd_slice) with OpenMP/BLAS. |
| matlab/cpp_mex/README.md | Documents the new C++ MEX rewrite directory, entrypoints, layouts, and label conventions. |
| matlab/cpp_mex/mex/info_cd_slice_cpp.cpp | Adds C++ MEX adapter entrypoint for info_cd_slice_cpp. |
| matlab/cpp_mex/mex/info_cc_slice_cpp.cpp | Adds C++ MEX adapter entrypoint for info_cc_slice_cpp. |
| matlab/cpp_mex/mex/info_cc_slice_cpp_capi.c | Adds a classic C-API MEX wrapper variant for info_cc_slice_cpp. |
| matlab/cpp_mex/mex/gcmi_cpp_runtime_probe.cpp | Adds OpenMP runtime probe MEX entrypoint. |
| matlab/cpp_mex/mex/gcmi_cpp_ping.cpp | Adds build/runtime identity probe MEX entrypoint. |
| matlab/cpp_mex/mex/gcmi_cpp_omp_probe.cpp | Adds OpenMP probe MEX entrypoint. |
| matlab/cpp_mex/mex/gcmi_cpp_blas_probe.cpp | Adds BLAS/LAPACK probe MEX entrypoint. |
| matlab/cpp_mex/include/gcmi_mex_utils.hpp | Adds shared BLAS/LAPACK + utility helpers for the C++ kernels. |
| matlab/cpp_mex/include/gcmi_mex_adapter_utils.hpp | Adds MATLAB C++ MEX adapter utilities (argument checking, label parsing). |
| matlab/cpp_mex/include/gcmi_kernels.hpp | Declares the C++ kernel APIs used by MEX entrypoints/bridges. |
| FUNCTIONALITY.md | Updates repo functionality inventory to include matlab/cpp_mex and label conventions. |
| buildfile.m | Adds MATLAB buildtool tasks to compile/test/bench/package the C++ MEX runtime. |
| benchmarks/run_matlab_benchmarks.m | Adds optional path injection, label encoding support, and optimized-data preparation for alternative backends. |
| benchmarks/README.md | Documents running benchmarks with the new C++ MEX backend path/label encoding options. |
| benchmarks/cpp_mex_default_function_map.m | Adds a default function-name map for the new C++ MEX backend. |
| .gitignore | Ignores matlab/cpp_mex/bin/ build outputs and local scratch/settings artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'lapack_library', cfg.LapackLibrary, ... | ||
| 'targets', {requested}, ... | ||
| 'generated_at', char(datetime('now', TimeZone='local', Format='yyyy-MM-dd''T''HH:mm:ssXXX'))); | ||
| fid = fopen(fullfile(cfg.OutputDir, 'build-info.json'), 'w'); |
There was a problem hiding this comment.
fopen() can return -1 on failure (e.g., permissions, missing directory). As written, fclose(-1) in the onCleanup will error and jsonencode output will be lost. Please check fid >= 0 and raise a clear error if the build-info.json cannot be opened for writing.
| fid = fopen(fullfile(cfg.OutputDir, 'build-info.json'), 'w'); | |
| buildInfoPath = fullfile(cfg.OutputDir, 'build-info.json'); | |
| [fid, msg] = fopen(buildInfoPath, 'w'); | |
| if fid < 0 | |
| error('gcmi_cpp_mex_compile:BuildInfoOpenFailed', ... | |
| 'Unable to open "%s" for writing: %s', buildInfoPath, msg); | |
| end |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 39 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| gcmi::info_cc_slice( | ||
| x, | ||
| static_cast<gcmi::mwSize>(ntrl), | ||
| static_cast<gcmi::mwSize>(xdim), | ||
| static_cast<gcmi::mwSize>(npage), | ||
| y, | ||
| static_cast<gcmi::mwSize>(ydim), | ||
| static_cast<gcmi::mwSize>(threads), | ||
| output); | ||
| *error_message = nullptr; | ||
| } catch (const std::exception& ex) { | ||
| *error_message = ex.what(); | ||
| } catch (...) { | ||
| *error_message = "unknown error"; | ||
| } |
There was a problem hiding this comment.
gcmi_info_cc_slice_bridge sets *error_message = ex.what(), but the std::exception object is destroyed when leaving the catch block, so the returned pointer can dangle. The caller (info_cc_slice_cpp_capi.c) then uses this pointer in mexErrMsgIdAndTxt, which is undefined behavior and can crash or print garbage. Store the message in storage that outlives the function call (e.g., a static/thread_local std::string whose c_str() is returned, or copy into a caller-owned buffer) before returning the pointer.
| const auto nTrials = dims[0]; | ||
| const auto nPages = dims[1]; | ||
| auto out = factory_.createArray<double>({nTrials, nPages}); | ||
| auto normalized = gcmi::copnorm_slice_kernel(raw_data(x), nTrials, nPages, threads); | ||
| if (!normalized.empty()) { | ||
| std::copy(normalized.begin(), normalized.end(), raw_data(out)); | ||
| } |
There was a problem hiding this comment.
This MEX wrapper allocates a full std::vector<double> via copnorm_slice_kernel(...) and then copies it into a newly allocated MATLAB array. For large inputs this doubles peak memory and adds an extra pass over the data. Consider refactoring the kernel to write directly into the MATLAB output buffer (or add an overload that takes an output pointer) to avoid the extra allocation/copy.
| function i_rmdir_if_exists(pathname) | ||
| if exist(pathname, 'dir') | ||
| rmdir(pathname, 's'); | ||
| end | ||
| end |
There was a problem hiding this comment.
i_rmdir_if_exists is defined but never called anywhere in this file, which adds dead code to a small regression harness. Either remove it, or add the intended call site so the helper is exercised (and consider adding a regression check around the behavior it was meant to support).
| repoRoot = fileparts(fileparts(fileparts(mfilename('fullpath')))); | ||
| addpath(repoRoot); | ||
| addpath(fullfile(repoRoot, 'benchmarks')); | ||
| setup_gcmi(); | ||
|
|
There was a problem hiding this comment.
This test runner now unconditionally adds benchmarks/ to the MATLAB path, but the file doesn’t appear to call any benchmark helpers (it defines its own assertions locally). If this path isn’t required, consider removing it to avoid path pollution/shadowing and to keep the regression harness minimal.
| p.addParameter('Compile', true, @(x) islogical(x) && isscalar(x)); | ||
| p.addParameter('LegacyPath', fullfile(fileparts(fileparts(fileparts(fileparts(mfilename('fullpath'))))), 'extern', 'gcmi_mex'), @(x) ischar(x) || isstring(x)); | ||
| p.parse(varargin{:}); |
There was a problem hiding this comment.
LegacyPath default goes up four directories from matlab/cpp_mex/tooling/gcmi_cpp_mex_test.m, which appears to land above the repository root. Given the repo layout used elsewhere (gcmi_cpp_mex_config goes up three levels to reach RepoRoot), this default likely points to a non-existent extern/gcmi_mex and silently disables legacy cross-checks. Consider deriving the legacy path from cfg.RepoRoot (or only walking up three levels) so the default matches the actual repo root.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c2db91abb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const auto xDims = x.getDimensions(); | ||
| const auto yDims = y.getDimensions(); | ||
| if (xDims.size() != 3 || xDims[0] != ntrl || xDims[1] != xdim) { |
There was a problem hiding this comment.
Accept single-page inputs in info_cc_slice_cpp
The shape guard hard-requires xDims.size() == 3, but MATLAB collapses trailing singleton dimensions, so valid one-page input [Ntrl, Xdim, 1] is commonly presented as a 2-D array [Ntrl, Xdim]. In that case this now throws info_cc_slice_cpp:shape instead of computing a single output page, which breaks single-slice workflows. Please treat 2-D input as Npage = 1 here.
Useful? React with 👍 / 👎.
| const auto threads = std::max<gcmi::mwSize>(1, scalar_to_size(inputs[5], "Nthread")); | ||
|
|
||
| const auto xDims = x.getDimensions(); | ||
| if (xDims.size() != 3 || xDims[0] != xdim || xDims[1] != ntrl) { |
There was a problem hiding this comment.
Accept single-page inputs in info_cd_slice_cpp
This check similarly requires xDims.size() == 3, so a valid single-page array [Xdim, Ntrl, 1] that MATLAB represents as 2-D [Xdim, Ntrl] is rejected with info_cd_slice_cpp:shape. That makes the new discrete MEX path unusable for Npage == 1 calls even though the underlying kernel can operate on one page.
Useful? React with 👍 / 👎.
| output); | ||
| *error_message = nullptr; | ||
| } catch (const std::exception& ex) { | ||
| *error_message = ex.what(); |
There was a problem hiding this comment.
Preserve exception message lifetime in C bridge
The bridge writes ex.what() into *error_message and returns, but that pointer is owned by the exception object and becomes invalid when the catch block exits. The C API wrapper then reads that dangling pointer in mexErrMsgIdAndTxt, which can produce corrupted messages or undefined behavior on runtime-error paths. Store a durable copy of the message (or emit the MATLAB error before returning) instead.
Useful? React with 👍 / 👎.
No description provided.