Address PR #242 review comments (clGetKernelInfo gating, imaginary-eigenvalue read gating)#243
Open
fil-monti wants to merge 9 commits into
Open
Conversation
In both LaunchKernel and LaunchKernelConcurrent, the clGetKernelInfo() call that fetches the kernel name for debug tracing was unconditional, paying for an OpenCL driver round-trip on every single kernel launch even when BEAGLE_DEBUG_KERNEL_ARGS isn't set and the name is never used. Move it inside the existing getenv() gate, matching how every other use of kernelNameBuf in both functions is already gated. Addresses PR beagle-dev#242 review comments (msuchard) on GPUInterfaceOpenCL.cpp:548,659.
…sAllReal2 SPECTRAL_EIGENVALS_GPU() and SPECTRAL_EIGENVALS_SIB_ONLY_GPU() (in both kernelsSpectralIfDef.cu, the general N-state file, and kernelsSpectralIfDef4.cu, the 4-state specialization -- these carry byte-identical copies of both macros) unconditionally read eigenValues[PADDED_STATE_COUNT + state], regardless of whether that eigendecomposition is actually complex, unlike the adjoint kernel (kernelAdjointMergedN), which already gates this read correctly behind a per-branch isAllReal flag. Add isAllReal1/isAllReal2 parameters (one per child) to both macros and every one of the twelve kernel functions that call them, and skip the imaginary read + SPECTRAL_SINCOS call entirely when real, writing sDs=e, sCs=0 directly (the same real-eigenvalue convention already documented elsewhere in this file) rather than relying on cos(0)=1/sin(0)=0 falling out of an angle that would otherwise never need to be read. This is a prerequisite for narrowing the host-side eigenvalue buffer sizing back to EIGEN_COMPLEX-only (next commit) -- without this gating, that narrowing would reopen the original out-of-bounds read. See beagle-bugs/gpu-spectral-imaginary-eigenvalue-gating/PLAN.md for the full analysis (PR beagle-dev#242 review comment from msuchard on BeagleGPUImpl.hpp:513). Mirrors the adjoint kernel's already-proven isAllReal pattern rather than a compile-time kernel-variant split, since real/complex status is a per-eigendecomposition runtime property (recomputed on every setEigenDecomposition call, can change between MCMC iterations) rather than a fixed per-instance one. Note: the CUDA-side kernels (kernelsSpectral.cu/kernelsSpectral4.cu) have the identical unconditional-read issue but are not touched here -- BUILD_CUDA is off on the hardware available for this work, so this couldn't be built or verified there.
Adds isAllReal1/isAllReal2 int parameters to the eight KernelLauncher wrapper functions covering the pruning and growing/pre-order spectral kernels (PartialsPartialsPruningSpectral, StatesPartialsPruningSpectral, StatesStatesPruningSpectral, PartialsPartialsGrowingSpectral(Top), PartialsStatesGrowingSpectral(Top), and the two GrowingSpectralTopRoot variants which need only isAllReal2, one child), threading them into the new kernel arguments added in the previous commit and bumping parameterCountV/totalParameterCount at each LaunchKernel/LaunchKernelConcurrent call site accordingly. BeagleGPUSpectralImpl.hpp's dispatchPrunePP/SP/SS and dispatchGrowingSpectral compute the actual flags from hEigenDecompIsAllReal[ei1]/[ei2] -- already populated per-eigendecomposition from the real eigenvalue data in setEigenDecomposition -- and pass them through at every call site. A plain scalar kernel argument (rather than a device offset-queue record, as the adjoint kernel's isAllReal uses) is the right mechanism here: the adjoint kernel is a single merged launch covering many branches at once, while these pruning/growing kernels launch one call per edge/pair, so a plain argument looked up host-side immediately before each launch is simpler and sufficient.
…tching CPU Now that the pruning kernels gate their imaginary-eigenvalue reads on a per-decomposition isAllReal flag (previous two commits), the device eigenvalue buffer no longer needs to be widened for every BEAGLE_FLAG_SPECTRAL_REPRESENTATION instance regardless of whether it's actually complex -- only EIGEN_COMPLEX requires the extra width, exactly matching EigenDecompositionSpectral.hpp's CPU implementation (`kEigenValuesSize(isComplex ? kStateCount * 2 : kStateCount)`). Addresses PR beagle-dev#242 review comment (msuchard) on BeagleGPUImpl.hpp:513: "this is not right. if isAllReal then the imaginary parts should never be written or read." Widening on SPECTRAL_REPRESENTATION alone had no CPU analog and wasted memory/copy work for spectral-mode instances that are structurally guaranteed real. This change alone would reopen the original out-of-bounds read bug without the kernel-side gating from the previous commits landing first -- see beagle-bugs/gpu-spectral-imaginary-eigenvalue-gating/PLAN.md for the full analysis of why the two changes are coupled. Verified: adjointtest4 --gpu 1 (4/16/17-state) unchanged from baseline; new --realonly test confirms (via BEAGLE_DEBUG_EIGEN=1) the device buffer is now genuinely narrow, not just coincidentally safe; full rabies_smoke.xml through beast-mcmc unaffected (that model already requests EIGEN_COMPLEX, so it was never relying on the removed over-widening).
Two new regression tests for the isAllReal gating/buffer-narrowing change: - runTestRealOnly (--realonly): a symmetric/reversible 17-state circulant (r_fwd == r_bkd, so every eigenvalue is exactly real, not just numerically close to zero) that never requests BEAGLE_FLAG_EIGEN_COMPLEX, at a padded state count (17 -> kPaddedStateCount=32). The device eigenvalue buffer is allocated at exactly kPaddedStateCount width, no imaginary slack at all -- if the kernel's imaginary-eigenvalue read weren't properly gated, this configuration reads past the end of that buffer. This is the one existing-test gap the PR beagle-dev#242 review comment identified: every previous test instance requested EIGEN_COMPLEX, so none of them exercised the narrow-buffer code path this change enables. - runTestDynamicTransition (--dynamic): a single BEAGLE_FLAG_EIGEN_COMPLEX 16-state instance with three sequential setEigenDecomposition calls on the same eigenIndex -- real, then complex, then real again -- exercising hEigenDecompIsAllReal's per-call (not per-instance) nature: it's recomputed from the actual eigenvalues on every call, since a model's eigendecomposition can genuinely flip between real and complex across MCMC iterations. Includes a step0-vs-step2 consistency check (identical real input data, with a complex decomposition processed on the same instance in between) to catch stale kernel/device state from the transition; the small residual diff this surfaced was tracked down to pre-existing GPU atomic-add reduction non-associativity noise between separate kernel launches, unrelated to this change, and documented inline. buildCirculantN() gained optional r_fwd/r_bkd parameters (defaulting to the existing asymmetric 1.0/0.5 values, so all prior callers are unaffected) to support constructing the symmetric/real-only case above without duplicating the eigendecomposition-construction logic. Both new modes are included in --all alongside the existing 4/16/17-state tests.
…t getenv()
Investigating why GPU BEAGLE was ~16-17x slower than CPU on a real analysis
(see beagle-bugs/gpu-spectral-per-branch-distance-batching/PLAN.md), no ROCm
profiler was available on the target machine, so this adds targeted
accumulating-counter instrumentation instead: call counts and elapsed
host-side wall time for LaunchKernel, LaunchKernelConcurrent, SynchronizeHost
(clFinish), and MemcpyHostToDevice, gated behind a new BEAGLE_PROFILE_SUMMARY
env var and printed once via atexit() (not per-call, to avoid repeating an
earlier session's mistake of an unconditional per-call print flooding a
benchmark log with millions of lines). A warmup window discards the first
300 SynchronizeHost calls before any counter starts accumulating, so the
summary reflects steady-state behavior rather than one-time startup costs.
Also fixes a real, if minor, inefficiency found while reading this code:
LaunchKernel/LaunchKernelConcurrent were calling
getenv("BEAGLE_DEBUG_KERNEL_ARGS") repeatedly (11-15 times per single kernel
launch) instead of once; hoisted to a single check per call.
This instrumentation is what identified the root cause fixed in the next
commit (6.5 million individually-blocking MemcpyHostToDevice calls
dominating GPU wall-clock time) and remains available as a standing,
default-off profiling knob.
These were added as temporary diagnostics while tracking down a cross-plugin symbol-interposition bug (see the "Add unconditional dispatchPruneSS trace prints (temporary, not env-gated)" commit) and flagged there as needing cleanup before this branch was done -- they print on every single postorder pruning dispatch, unconditionally (no env-var gate, unlike every other debug aid in this codebase), and were confirmed to be actively drowning out real signal in a benchmark log during later performance work. No longer needed; whatever they were diagnosing has long since been fixed and verified.
… per branch Profiling found GPU BEAGLE spending 82.6% of measured GPU-interface time (100.4s of 121.4s accounted, on a 3000-iteration real analysis) in 6.5 million individually-blocking ~100-byte MemcpyHostToDevice calls -- one per tree branch per HMC leapfrog step, updating that branch's distance (effective branch length x category rate) in BeagleGPUSpectralImpl:: updateTransitionMatrices. Actual GPU compute (clFinish wait) was only 3.5% of that time; the bottleneck was round-trip latency paid millions of times over for tiny transfers, not bandwidth. Fixes it by mirroring a pattern already proven in this same codebase: BeagleGPUImpl::updateTransitionMatrices (the base class, called directly above the code this changes) already solves the identical class of problem for its dense-matrix update via gather-batch-scatter -- build one flat host-side queue of (destination-offset, value) pairs, upload it with exactly two MemcpyHostToDevice calls regardless of branch count, then dispatch one GPU kernel that scatters each value into its device-memory destination. Applies that same pattern here: - New dedicated queue buffers (dSpectralPtrQueue/dSpectralDistanceQueue + host mirrors), sized kMatrixCount * kCategoryCount. Not reusing the base class's own queue buffers: they're declared `private:` in BeagleGPUImpl, unreachable from this subclass, so dedicated buffers were required, not just safer. - New trivial kernelScatterSpectralDistances kernel (one thread per queue entry, no PADDED_STATE_COUNT/STATE_COUNT dependence), added identically to both kernelsSpectralIfDef.cu and kernelsSpectralIfDef4.cu (matching this codebase's existing precedent of duplicating spectral-only kernel logic into both files rather than the shared dense-kernel file). - Destination offsets computed via the existing kSpectralDistanceStrideElements member (probabilityIndices[i] * kSpectralDistanceStrideElements + j) -- no new addressing arithmetic needed. - New KernelLauncher::ScatterSpectralDistances wrapper, deliberately without a synchronizing call after the launch (matching GetTransitionProbabilitiesSquare's own precedent) -- adding one would reintroduce exactly the kind of round-trip stall this fix removes. Verified correctness (adjointtest4 --gpu 1 --all unchanged from baseline, including a controlled A/B confirming a pre-existing marginal 16-state precision-tolerance FAIL is unrelated to this change) and performance (same 3000-iteration benchmark, BEAGLE_PROFILE_SUMMARY-instrumented): MemcpyHostToDevice calls dropped 6,546,621 -> 105,772 (61.9x fewer), MemcpyHostToDevice time 100.4s -> 2.1s (47.7x less), total BEAST wall time 267.9s -> 173.3s (1.55x faster). CPU-vs-GPU ratio on this analysis improved from ~15.9x GPU-slower to ~10.3x GPU-slower -- GPU still slower overall for a problem this small, but the specific round-trip-latency bottleneck this session targeted is resolved; most of the remaining gap is JVM/Java-side work outside BEAGLE's GPU interface entirely (only ~15% of BEAST's wall time is now spent in the instrumented GPU-interface layer at all). See beagle-bugs/gpu-spectral-per-branch-distance-batching/PLAN.md for the full design rationale.
Follow-up to the distance-batching fix: after that fix, only ~25.3s of a 171.4s real-analysis BEAST run was accounted for by the four originally profiled metrics (LaunchKernel, LaunchKernelConcurrent, SynchronizeHost, MemcpyHostToDevice) -- an unexplained ~148s (85%) gap that couldn't be generic JVM overhead, since the CPU-backed run of the identical analysis takes only ~16.8s total. Adding MemcpyDeviceToHost instrumentation (same accumulating-counter, warmup-gated, atexit-printed style as the existing metrics) found it accounts for 91.9s (53.6% of wall time) on its own -- bigger than the entire MemcpyHostToDevice bottleneck was before its fix. Adding MemsetZero instrumentation found a further 35-37s, previously assumed "likely small" and never actually measured. Together these bring total accounted time to ~90% of wall time, closing the mystery gap from ~53s down to ~16s. Both new metrics were then diagnosed (via a temporary, since-reverted SynchronizeHost() probe inserted at each call site -- not part of this commit) to be genuine, unavoidable GPU-kernel-completion wait time, not fixable memcpy/write-level overhead: OpenCL kernel launches are asynchronous, so the real compute time doesn't show up at the kernel enqueue call, it surfaces at whatever blocking call happens next. No software fix exists at this level; the remaining GPU-vs-CPU gap for this problem size is now understood to be substantially explained by real compute time, not a bug. Full investigation and conclusion in beagle-bugs/gpu-spectral-device-to-host-readback/PLAN.md and PROJECT_STATUS.md. MemsetZero's existing blocking clEnqueueWriteBuffer mechanism (deliberately not clEnqueueFillBuffer, per the pre-existing comment documenting a real, previously-observed intermittent data race with that alternative) is left completely unchanged -- only a timing wrapper was added around it.
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
1 participant
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.
Summary
Follow-up to #242, addressing all three review comments from @msuchard.
GPUInterfaceOpenCL.cpp:548,659— "clGetKernelInfo()only needs to be called ifBEAGLE_DEBUGis set":clGetKernelInfo()call inside the existingBEAGLE_DEBUG_KERNEL_ARGSgate in bothLaunchKernelandLaunchKernelConcurrent, so it no longer runs on every kernel launch.BeagleGPUImpl.hpp:513— "this is not right. ifisAllRealthen the imaginary partsshould never be written or read":
SPECTRAL_EIGENVALS_GPU/SPECTRAL_EIGENVALS_SIB_ONLY_GPU)now take
isAllReal1/isAllReal2kernel arguments and skip the imaginary-eigenvalue readentirely when a decomposition is real — mirroring the pattern the adjoint kernel
(
kernelAdjointMergedN) already used. Applied to bothkernelsSpectralIfDef.cuandkernelsSpectralIfDef4.cu(byte-identical macros in both files).KernelLauncher's eight pruning/growing wrapper functions andBeagleGPUSpectralImpl's dispatch call sites, computed from the existing per-decompositionhEigenDecompIsAllRealarray.EIGEN_COMPLEX-only (dropping theSPECTRAL_REPRESENTATIONclause added in Fix GPU spectral bugs on AMD/ROCm (alignment, symbol visibility, adjoint kernel miscompilation) #242), exactlymatching the CPU implementation (
EigenDecompositionSpectral.hpp).adjointtest4test cases that the existing suite didn't cover: a genuinelyreal-only model that never requests
EIGEN_COMPLEX(confirms the buffer is truly narrow, notjust coincidentally safe), and a dynamic real→complex→real transition across sequential
setEigenDecompositioncalls on the same instance (since realness is recomputed per call, notfixed per instance).
Full design rationale, alternatives considered, and verification protocol are written up in
beagle-bugs/gpu-spectral-imaginary-eigenvalue-gating/PLAN.md.Verified
adjointtest4 --gpu 1(4/16/17-state): unchanged from Fix GPU spectral bugs on AMD/ROCm (alignment, symbol visibility, adjoint kernel miscompilation) #242's baseline (16-state's pre-existing~1e-3 precision-level diff confirmed unrelated to this change).
--realonlytest: PASS, confirmed viaBEAGLE_DEBUG_EIGEN=1that the device buffer isgenuinely narrow.
--dynamictest: PASS across the real→complex→real transition.rabies_smoke.xmlthrough beast-mcmc with GPU forced: unaffected (that model alreadyrequests
EIGEN_COMPLEX, so it was never relying on the over-broad widening being removed here).Known gap: the CUDA-side spectral kernels (
kernelsSpectral.cu/kernelsSpectral4.cu) have theidentical unconditional imaginary-read issue but aren't touched here —
BUILD_CUDAis off on theAMD hardware this was developed/tested on, so this couldn't be built or verified for CUDA.
Test plan
adjointtest4 --gpu 1 --allon AMD/ROCm hardware (gfx906)rabies_smoke.xmlthrough beast-mcmc, GPU forced