Repository navigation
Ada - #686
Open
jeffhammond wants to merge 24 commits into
Open
Ada#686jeffhammond wants to merge 24 commits into
jeffhammond wants to merge 24 commits into
Conversation
… heap-allocate ADA/ had nstream (+ a heap-allocated nstream_array variant) and transpose, but was missing p2p and stencil -- the other two kernels every comparable language port (JAVA, Kotlin, ...) implements. Added both, following the existing Ada style in this directory (Ada.Real_Time Clock-based timing with the "microseconds-per-iteration" Time_Span division trick already used by nstream.adb/transpose.adb, Argument_Count-based CLI parsing with defaults, same closed-form validation approach as the other ports). Running the existing kernels end-to-end surfaced two real, pre-existing bugs, not just missing coverage: - transpose.adb never actually transposed anything: its timed loop was `for I in 1..Order loop A(I,J) := A(I,J) + B(I,J); end loop` with J fixed at its declared-but-never-reassigned value of 1 -- a 1-D loop over a single column, no cross-indexing at all. It also initialized A to all zeros and validated by checking A stayed at zero, which cannot correspond to any real transpose algorithm. Confirmed by running it: it printed "Asum= 2.20000E+04" and never reached the timing output, i.e. validation always failed. Rewritten as a real B += A^T with the standard PRK closed-form checksum. - stencil (once written) failed validation for the grid pattern and larger radii, but not the default star/radius=2 case: Ada.Numerics. Real_Arrays.Real_Matrix is single-precision Float, and accumulating more terms per point (grid pattern, or larger radius) lost enough precision to exceed the 1.0E-8 tolerance. Every other language port of this kernel uses double precision. Fixed by defining a local Long_Float matrix type instead of Real_Matrix for p2p/stencil/ transpose, rather than loosening the tolerance. - Every one of p2p/stencil/transpose stack-allocates its matrices; at realistic benchmark sizes (e.g. 1000x1000 Long_Float = 8MB per array) that reliably blew the 8MB default stack (STORAGE_ERROR). Switched all three to heap allocation via an access type, the same pattern nstream_array.adb already established in this directory for exactly this reason. nstream.adb itself is left as the original small-array stack-based variant (works up to ~500K elements); nstream_array.adb remains the one to use for large vector lengths. Verified: `make` builds all 7 programs cleanly (gnatmake, no warnings on the new/changed files); ran every kernel across multiple sizes/ patterns (p2p at 1000x1000 and 4000x4000, transpose at 1000 and 2000, stencil at star/grid x radius 2/4 x multiple sizes) -- all validate, with rates in a consistent, physically sensible range for scalar Ada (p2p ~134 MFlops/s regardless of grid size, matching its inherently sequential dependency chain; transpose ~700 MB/s; stencil ~140-155 MFlops/s across all pattern/radius/size combinations). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion nstream.adb stack-declared its three arrays, which only worked up to ~500K elements (Length * 3 arrays * 4 bytes ~ 8MB default stack) before raising STORAGE_ERROR -- not a realistic size for a stream-bandwidth benchmark. nstream_array.adb already solved this with heap allocation via an access type (plus double precision, matching the convention now used by p2p/stencil/transpose too), so there was no reason to keep both: replaced nstream.adb's body with nstream_array.adb's approach (cleaning up the unused AvgTime variable and dead commented-out debug lines along the way) and removed nstream_array.adb. Verified: clean build, no compiler warnings. Ran nstream at 500K (the old stack limit), 1M (previously STORAGE_ERROR), 16M, and 100M elements -- all validate with consistent ~6.6-6.8 GB/s bandwidth, no more artificial size cap. Re-ran the rest of ADA/ (p2p, transpose, stencil star/grid) to confirm nothing else regressed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
These were CLI-parsing/Hello-World scratch files, not part of the 4-kernel PRK set (p2p, nstream, stencil, transpose). ADA/ now contains exactly the same kernels as the other comparable ports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CMake has no native Ada language support (no LANGUAGES Ada for project()/enable_language()), so ADA/CMakeLists.txt drives gnatmake directly via add_custom_command() -- the same approach this repo already uses elsewhere for build steps CMake can't natively express (e.g. common/Stencil/loop_gen code generation). gnatmake's own -D flag keeps all generated .o/.ali files in the CMake build directory, so the ADA/ source tree stays untouched by an out-of-source build, same as every other language here. Custom targets are named ada_<name> (unique across the combined root project, same reasoning as every other language's CMakeLists.txt), but since these are custom targets/commands rather than real add_executable() targets, there's no OUTPUT_NAME to reconcile -- the produced binary is simply named <name> directly and CTest invokes it by that known path. Verified: standalone `cd ADA && cmake -S . -B build && cmake --build build` builds all 4 kernels cleanly with the ADA/ source directory left untouched; all 6 CTest cases (nstream, p2p, transpose, stencil x star/grid/radius-4) pass. Also verified combined at the repo root with all other languages enabled: clean configure (no target-name collisions), full build succeeds, and the combined CTest run is 203/209 passing with the same 4 pre-existing unrelated failures as before this change (no regressions). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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.
If this pull request is fixing a bug, please link the associated issue.
The rest of this template does not apply.
If this pull request is providing a new implementation of the PRKs,
please use the following template.
Note that checking all of the boxes is not required.
New PRK implementation checklist
Which kernels are implemented?
Documentation and build examples
If your implementation uses a new programming model that is not
ubiquitious (i.e. included in the system compiler on most systems)
then you need to provide a link to the appropriate documentation
for a new user to install it, etc.
We strongly recommend that you add the appropriate features
to
make.defs.${toolchain}if appropriate.Do you certify that your contribution is made in good faith and does not attempt to introduce any negative behavior into this project?