Conversation
…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 stan-dev#3304
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
Thanks! Though I think I would prefer if we used |
Move the ref_type_t<Args> wrapping out of the call sites in reduce_sum and reduce_sum_static and into the recursive_reducer's args_tuple_ member. Both prim arithmetic and rev var specializations now own the materialized Eigen expression by typing args_tuple_ as std::tuple<ref_type_t<Args>...>; the call sites pass plain Args... and the implicit conversion materializes the temporary at construction. Per @SteveBronder review on stan-dev#3305.
|
@SteveBronder Pushed f73f114.
Verified locally with Jenkins i think will exercise the rest including the mix tests and the perf benchmarks. |
The recursive_reducer's args_tuple_ stores ref_type_t<Args>... so Eigen expression-template args (e.g. M.row(0)) materialize at storage time. The rev specialization's outer operator() was still calling save_varis(args...) on the raw expressions before constructing the worker, then materializing in the args_tuple_ initialization, walking the underlying expression twice. ExpressionTestRev.reduce_sum1 caught this: matrix2_expr3_counter == 2 vs expected <= 1. Materialize args... once at the function entry into a local std::tuple<ref_type_t<Args>...> args_refs, then route count_vars, save_varis, and worker construction through stan::math::apply on args_refs. Subsequent calls see plain matrices, so the counter increments once (in args_refs initialization) and not again. The prim specialization needs no change: its outer operator() does not call count_vars or save_varis on the shared args, so args_tuple_ initialization is the only evaluation. Closes stan-dev#3304.
|
@SteveBronder Pushed Locally the failing expression test passes and the broader unit suite stays green: |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@SteveBronder anything else I should do for this? Lmk |
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: 52 bits physical, 57 bits virtual CPU(s): 192 On-line CPU(s) list: 0-191 Thread(s) per core: 2 Core(s) per socket: 48 Socket(s): 2 NUMA node(s): 2 Vendor ID: AuthenticAMD CPU family: 25 Model: 17 Model name: AMD EPYC 9474F 48-Core Processor Stepping: 1 Frequency boost: enabled CPU MHz: 1494.606 CPU max MHz: 4114.4229 CPU min MHz: 1500.0000 BogoMIPS: 7189.04 Virtualization: AMD-V L1d cache: 3 MiB L1i cache: 3 MiB L2 cache: 96 MiB L3 cache: 512 MiB NUMA node0 CPU(s): 0-47,96-143 NUMA node1 CPU(s): 48-95,144-191 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: Not affected 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; Enhanced / Automatic IBRS; IBPB conditional; STIBP always-on; PBRSB-eIBRS Not affected; BHI Not affected Vulnerability Srbds: Not affected Vulnerability Tsa: Mitigation; Clear CPU buffers 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 amd_lbr_v2 nopl xtopology nonstop_tsc cpuid extd_apicid aperfmperf rapl pni pclmulqdq monitor ssse3 fma cx16 pcid 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 perfmon_v2 ibrs ibpb stibp ibrs_enhanced vmmcall fsgsbase bmi1 avx2 smep bmi2 erms invpcid cqm rdt_a avx512f avx512dq rdseed adx smap avx512ifma clflushopt clwb avx512cd sha_ni avx512bw avx512vl xsaveopt xsavec xgetbv1 xsaves cqm_llc cqm_occup_llc cqm_mbm_total cqm_mbm_local user_shstk avx512_bf16 clzero irperf xsaveerptr rdpru wbnoinvd amd_ppin cppc arat npt lbrv svm_lock nrip_save tsc_scale vmcb_clean flushbyasid decodeassists pausefilter pfthreshold avic v_vmsave_vmload vgif x2avic v_spec_ctrl vnmi avx512vbmi umip pku ospke avx512_vbmi2 gfni vaes vpclmulqdq avx512_vnni avx512_bitalg avx512_vpopcntdq la57 rdpid overflow_recov succor smca fsrm flush_l1d debug_swap 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 |
5 participants
Summary
Closes #3304.
reduce_sumwraps its shared arguments withref_type_t<Args&&>...before handing them to TBB so that Eigen expression-template temporaries (e.g.M.row(0),matrix.col(i)) materialize instead of being held as references to a destroyed temporary.reduce_sum_static'sSTAN_THREADSbranch was not updated in the same pass and still forwarded plainArgs....A caller that passes a temporary Eigen expression as a shared argument to
reduce_sum_staticcan therefore end up with a dangling reference once the temporary falls out of scope at the call site.Fix
stan/math/prim/functor/reduce_sum_static.hpp:This exactly mirrors the wrapping already in place in
reduce_sum.hpp(see line 208).Notes
ref_type_tis already in scope viastan/math/prim/meta.hpp(already#included).ReduceFunction()(noreduce_sum_implinstantiation) or returns zero.Closes #3304