Make sure reduce_sum_static only evaluates inputs once - #3384
Merged
Conversation
SteveBronder
force-pushed
the
fix/3304-reduce-sum-static-ref-type-minimal
branch
from
September 1, 2026 19:18
f890f56 to
7789a54
Compare
Member
Huh, glad we have this check, would've totally missed it... |
Collaborator
Author
|
Thank you past Steve 🙏 |
SteveBronder
force-pushed
the
fix/3304-reduce-sum-static-ref-type-minimal
branch
from
September 1, 2026 19:43
7789a54 to
a7425e7
Compare
Member
|
Locally, one of the new tests fails: |
…pper Regression tests for #3304. Both fail against develop when built with STAN_THREADS; without threads reduce_sum_static bypasses reduce_sum_impl entirely, so the assertions hold trivially there. prim: passing an Eigen expression template as a shared argument to reduce_sum_static re-evaluates it once per TBB worker (4 reads with grainsize 1 over 4 terms) instead of once. reduce_sum, which already wraps Args with ref_type_t, reads it once. rev: the same call does not compile. The unwrapped CwiseUnaryOp reaches save_varis, which uses single-index coeff(i) on an expression without LinearAccessBit, tripping an Eigen static assertion.
…n expressions
reduce_sum has been wrapping shared Args with ref_type_t<Args&&>... since
v4.8+ so that Eigen expression-template temporaries (e.g. M.row(0),
matrix.col(i)) materialize before being handed off to TBB's parallel
reducer. reduce_sum_static's STAN_THREADS branch was not updated in the
same pass and still forwarded plain Args..., so callers passing a
temporary Eigen expression as a shared argument could get a dangling
reference once the temporary went out of scope.
Mirror the reduce_sum wrapping exactly:
internal::reduce_sum_impl<..., ref_type_t<Args&&>...>()(...)
No other changes. ref_type_t is already in scope via
stan/math/prim/meta.hpp (already included).
Closes #3304
SteveBronder
force-pushed
the
fix/3304-reduce-sum-static-ref-type-minimal
branch
from
September 1, 2026 20:33
a7425e7 to
435547c
Compare
Collaborator
Author
|
Ah I was running locally with only stan threads on. Fixed for stan threads being on and off |
WardBrian
approved these changes
Sep 1, 2026
Contributor
Jenkins Console Log Machine informationDistributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focal CPU: Architecture: x86_64 CPU op-mode(s): 32-bit, 64-bit Byte Order: Little Endian Address sizes: 43 bits physical, 48 bits virtual CPU(s): 256 On-line CPU(s) list: 0-255 Thread(s) per core: 2 Core(s) per socket: 64 Socket(s): 2 NUMA node(s): 2 Vendor ID: AuthenticAMD CPU family: 23 Model: 49 Model name: AMD EPYC 7742 64-Core Processor Stepping: 0 Frequency boost: enabled CPU MHz: 1497.196 CPU max MHz: 3416.0681 CPU min MHz: 1500.0000 BogoMIPS: 4491.55 Virtualization: AMD-V L1d cache: 4 MiB L1i cache: 4 MiB L2 cache: 64 MiB L3 cache: 512 MiB NUMA node0 CPU(s): 0-63,128-191 NUMA node1 CPU(s): 64-127,192-255 Vulnerability Gather data sampling: Not affected Vulnerability Indirect target selection: Not affected Vulnerability Itlb multihit: Not affected Vulnerability L1tf: Not affected Vulnerability Mds: Not affected Vulnerability Meltdown: Not affected Vulnerability Mmio stale data: Not affected Vulnerability Reg file data sampling: Not affected Vulnerability Retbleed: Mitigation; untrained return thunk; SMT enabled with STIBP protection Vulnerability Spec rstack overflow: Mitigation; Safe RET Vulnerability Spec store bypass: Mitigation; Speculative Store Bypass disabled via prctl Vulnerability Spectre v1: Mitigation; usercopy/swapgs barriers and __user pointer sanitization Vulnerability Spectre v2: Mitigation; Retpolines; IBPB conditional; STIBP always-on; RSB filling; PBRSB-eIBRS Not affected; BHI Not affected Vulnerability Srbds: Not affected Vulnerability Tsa: Not affected Vulnerability Tsx async abort: Not affected Vulnerability Vmscape: Mitigation; IBPB before exit to userspace Flags: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt pdpe1gb rdtscp lm constant_tsc rep_good nopl xtopology nonstop_tsc cpuid extd_apicid aperfmperf rapl pni pclmulqdq monitor ssse3 fma cx16 sse4_1 sse4_2 x2apic movbe popcnt aes xsave avx f16c rdrand lahf_lm cmp_legacy svm extapic cr8_legacy abm sse4a misalignsse 3dnowprefetch osvw ibs skinit wdt tce topoext perfctr_core perfctr_nb bpext perfctr_llc mwaitx cpb cat_l3 cdp_l3 hw_pstate ssbd mba ibrs ibpb stibp vmmcall fsgsbase bmi1 avx2 smep bmi2 cqm rdt_a rdseed adx smap clflushopt clwb sha_ni xsaveopt xsavec xgetbv1 xsaves cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local clzero irperf xsaveerptr rdpru wbnoinvd amd_ppin arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold avic v_vmsave_vmload vgif v_spec_ctrl umip rdpid overflow_recov succor smca sev sev_es G++: g++ (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0 Copyright (C) 2019 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Clang: clang version 10.0.0-4ubuntu1 Target: x86_64-pc-linux-gnu Thread model: posix InstalledDir: /usr/bin |
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.
Summary
Fixes #3304 . Minimal alternative to #3305 which makes sure that expressions passed to
reduce_sum_staticare only evaluated once.Tests
Thanks @tmchow !
Checklist
Copyright holder: Trevin Chow & Steve Bronder
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested