Hi Dave, Sorry to bring you two more -- this is a follow-up about the last two patches from the original series, which I know aren't really your area.
I've attached v2-0007 and v2-0008. Both come from the original series: patch 7 is what's left of old patch 5 once Martin's ipa-cp part was merged out, and patch 8 is the gcov.cc part of old patch 4 that you said you couldn't speak to. > > > I can't speak to patch 5. Patch 7 passes a couple of 80-byte objects (wide_int/widest_int) by reference instead of copying them. To check how often that actually matters, I wrote a test file with lots of division, xor and min/max comparisons -- those are exactly the operations that reach these functions. I then traced a real build while compiling it: tens of thousands of hits from that one file alone. > > > "[PATCH 4/5] gcc, analyzer: drop std::move calls that have no > > > effect": > > > * the analyzer/*.cc changes look good to me > > > * I can't speak to the gcov.cc change Patch 8 drops a std::move that doesn't actually do anything: the value is a map key, which is always const, so it was already being copied either way. It's a clarity fix, not a speedup. Do you know who I should ask, or what the right next step is? Thanks, Linden On Tue, Oct 06, 2026 07:48 AM, David Malcolm <[email protected]> wrote: > On Mon, 2026-10-05 at 16:18 +1300, best xmg wrote: > > Hi Dave, > > > > Quick recap: this is the v2 series I sent on Sep 26 -- patches > > 0001-0004 and 0006. 0001-0003 are unchanged from what you approved. > > 0004 now has only the analyzer and xml.cc hunks; I dropped the > > gcov.cc and canvas.cc hunks. 0006 is the array2::set overload you > > suggested, so the std::move in canvas::paint now really moves. > > (Martin has already pushed the ipa-cp patch.) > > > > Just a gentle ping on these -- let me know if anything needs > > changing, > > or if you're able to push them. > > Thanks for the reminder. I've now pushed these for you to trunk as > r17-4901-gf9b60a8bd7569f through r17-4906-g2633f491aedadd. Please let > me know if I've missed anything. > > Thanks again for the patches (and the speedup!) > Dave > > > > > > Thanks, > > Linden > > > > On Sat, Sep 26, 2026 11:21 PM, best xmg <[email protected]> wrote: > > > Hi Dave, > > > > > > Thanks for the review. Attached are v2-0004 and v2-0006. > > > > > > v2-0004-gcc-analyzer-drop-std-move-calls-that-have-no- > > > effect.patch.txt > > > now has only the analyzer and xml.cc hunks you approved. I dropped > > > the > > > gcov.cc and canvas.cc hunks; canvas.cc keeps its std::move. > > > > > > You are right about the canvas.cc change. > > > v2-0006-text-art-add-array2-set-overload-taking-an-rvalue- > > > el.patch.txt > > > is new, as you suggested: an array2::set overload taking element_t > > > &&, > > > so that std::move in canvas::paint now really moves. > > > > > > The series bootstrapped (x86_64-pc-linux-gnu, --enable- > > > checking=yes) > > > with no errors. > > > > > > Could you push patch 1-3 along with v2-0004 and v2-0006 for me? > > > I don't have commit access. > > > > > > If anything is not going well, please tell me; I would really > > > appreciate > > > it. > > > > > > Thanks, > > > Linden > > > > > > On Fri, Sep 25, 2026 05:58 AM, David Malcolm <[email protected]> > > > wrote: > > > > On Thu, 2026-09-24 at 10:21 +1200, best xmg wrote: > > > > > Hi all, > > > > > > > > > > Reference and move semantics are a part of C++ that experienced > > > > > engineers > > > > > still get subtly wrong, so I audited the tree with clang-tidy's > > > > > performance-* checks, which trace whether a by-value parameter > > > > > is > > > > > ever > > > > > mutated or a std::move target is actually reachable -- the kind > > > > > of > > > > > exhaustive per-parameter check regular compiler warnings and > > > > > human > > > > > review > > > > > aren't built to do. Only patch 1 has a measured performance > > > > > number; > > > > > patches 2-5 are correctness/consistency fixes in the same > > > > > spirit with > > > > > no > > > > > individual perf claim. No functional change is intended > > > > > anywhere in > > > > > the > > > > > series. > > > > > > > > > > Patch 1 removes a needless deep-copy of program_state (which > > > > > owns a > > > > > whole > > > > > region_model) when -fanalyzer creates an exploded_node, by > > > > > giving > > > > > program_state a move-assignment operator and threading the move > > > > > through > > > > > point_and_state and exploded_node. > > > > > > > > > > Testing: > > > > > - x86_64-pc-linux-gnu, --enable-languages=c,c++,lto, --disable- > > > > > bootstrap. > > > > > - check-gcc analyzer.exp/tree-ssa.exp/ipa.exp on current trunk > > > > > (492fbdbf99d): 16820+10537 pass. Same 2 pre-existing failures > > > > > as > > > > > unpatched trunk, unrelated to this series: > > > > > * omp-parallel-for-1.c: -Wanalyzer-symbol-too-complex depth > > > > > boundary. > > > > > * ssa-sink-18.c: known r17-3756 regression (unrelated ivopts > > > > > change). > > > > > - Timing (patch 1 only): 8 interleaved `cc1 -fanalyzer` runs on > > > > > libiberty/cp-demangle.c, binary swapped each run: 63.98s -> > > > > > 60.10s > > > > > median (6.06% faster), no overlap across all 8 pairs. > > > > > > > > Thanks, nice to see a measurable performance win in the analyzer > > > > for > > > > this. > > > > > > > > Did you try a bootstrap of GCC with the patches? > > > > > > > > Taking the patches one by one: > > > > > > > > "[PATCH 1/5] analyzer: avoid deep-copying program_state when > > > > creating > > > > an exploded_node": looks good to me. > > > > > > > > "PATCH 2/5] analyzer, diagnostics: add missing std::move for by- > > > > value > > > > sinks": looks good to me. > > > > > > > > "[PATCH 3/5] diagnostics: take HTML tag names as const char * in > > > > source-printing": looks good to me > > > > > > > > "[PATCH 4/5] gcc, analyzer: drop std::move calls that have no > > > > effect": > > > > * the analyzer/*.cc changes look good to me > > > > * I can't speak to the gcov.cc change > > > > * gcc/text-art/canvas.cc: looks like it would be better to > > > > implement an > > > > overload of text_art::array2::set that takes an element_t &&, and > > > > keep > > > > the std::move here, since copying styled_unichar instances is > > > > non- > > > > trivial > > > > * the xml.cc change looks good to me > > > > > > > > I can't speak to patch 5. > > > > > > > > Thanks again > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Full test logs and the benchmark script are available on > > > > > request. > > > > > > > > > > I don't have push access; posting for review. Happy to split > > > > > patch 1 > > > > > out on its own if that's preferred, or to fold 2-5 together -- > > > > > they're > > > > > independent of each other and of patch 1. > > > > > > > > > > The five patches are attached as plain-text files. > > > > > > > > > > linden (5): > > > > > analyzer: avoid deep-copying program_state when creating an > > > > > exploded_node > > > > > analyzer, diagnostics: add missing std::move for by-value sinks > > > > > diagnostics: take HTML tag names as const char * in source- > > > > > printing > > > > > gcc, analyzer: drop std::move calls that have no effect > > > > > range-op, tree-ssa-ccp, fold-const, ipa-cp: bind wide_int / > > > > > widest_int by > > > > > reference > > > > > > > > > > gcc/analyzer/access-diagram.cc | 2 +- > > > > > gcc/analyzer/engine.cc | 17 ++++++++++++----- > > > > > gcc/analyzer/exploded-graph.h | 16 ++++++++++++++++ > > > > > gcc/analyzer/infinite-loop.cc | 2 +- > > > > > gcc/analyzer/program-state.cc | 27 +++++++++++++++++++++++++++ > > > > > gcc/analyzer/program-state.h | 1 + > > > > > gcc/diagnostics/html-sink.cc | 4 ++-- > > > > > gcc/diagnostics/source-printing.cc | 27 ++++++++++++----------- > > > > > ---- > > > > > gcc/fold-const.cc | 9 +++++---- > > > > > gcc/fold-const.h | 8 ++++---- > > > > > gcc/gcov.cc | 2 +- > > > > > gcc/ipa-cp.cc | 9 ++++++--- > > > > > gcc/ipa-cp.h | 6 +++--- > > > > > gcc/range-op.cc | 10 +++++----- > > > > > gcc/text-art/canvas.cc | 2 +- > > > > > gcc/tree-ssa-ccp.cc | 5 +++-- > > > > > gcc/xml.cc | 2 +- > > > > > 17 files changed, 101 insertions(+), 48 deletions(-) > > > > > > > > > > Regards, > > > > > Linden > > > > > >
From b6fbd35dab76a505c93471b28c2e086f7edc2bc0 Mon Sep 17 00:00:00 2001 From: linden <[email protected]> Date: Sat, 26 Sep 2026 18:41:55 +1200 Subject: [PATCH 7/7] range-op, tree-ssa-ccp, fold-const: bind wide_int / widest_int by reference wide_int and widest_int are 40-130 byte inline-buffer types; several locals and by-value parameters across Ranger, CCP and fold-const copy one where only reading is needed. Bind these by const reference instead; no functional change. bit_value_binop's rotate arm keeps its copy since it reassigns 'shift'. gcc/ChangeLog: * range-op.cc (operator_div::wi_fold): Bind dividend_min/max and divisor_min/max as const wide_int & rather than copying. (operator_bitwise_xor::wi_fold): Likewise for new_lb. * tree-ssa-ccp.cc (valid_lattice_transition): Take old_val and new_val as const ccp_prop_value_t &. (bit_value_binop): Bind 'shift' as const widest_int & in the shift arm. * fold-const.h (minmax_from_comparison, fold_cmp_float_cst_p): Take widest_int / wide_int parameters by const reference. * fold-const.cc (minmax_from_comparison, fold_cmp_float_cst_p): Likewise. Signed-off-by: linden <[email protected]> --- gcc/fold-const.cc | 9 +++++---- gcc/fold-const.h | 8 ++++---- gcc/range-op.cc | 10 +++++----- gcc/tree-ssa-ccp.cc | 5 +++-- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc index 4c4b535d440..8d6ab29b5e4 100644 --- a/gcc/fold-const.cc +++ b/gcc/fold-const.cc @@ -198,8 +198,8 @@ static tree fold_negate_expr (location_t, tree); The form is "(exp0 CMP cst1) ? exp0 : cst2". */ tree_code minmax_from_comparison (tree_code cmp, tree exp0, - const widest_int cst1, - const widest_int cst2) + const widest_int &cst1, + const widest_int &cst2) { if (cst1 == cst2) { @@ -7113,9 +7113,10 @@ fold_real_zero_addition_p (const_tree type, const_tree arg, due to truncation/rounding. */ bool -fold_cmp_float_cst_p (wide_int lo, wide_int hi, enum tree_code cmp, +fold_cmp_float_cst_p (const wide_int &lo, const wide_int &hi, + enum tree_code cmp, const REAL_VALUE_TYPE *r, format_helper fmt, - wide_int i, signop isign) + const wide_int &i, signop isign) { REAL_VALUE_TYPE raw; REAL_VALUE_TYPE rnd; diff --git a/gcc/fold-const.h b/gcc/fold-const.h index ab03bf0aa35..437d8ece021 100644 --- a/gcc/fold-const.h +++ b/gcc/fold-const.h @@ -173,11 +173,11 @@ extern bool integer_valued_real_p (tree, int = 0); extern bool fold_real_zero_addition_p (const_tree, const_tree, const_tree, int); -extern bool fold_cmp_float_cst_p (wide_int lo, wide_int hi, +extern bool fold_cmp_float_cst_p (const wide_int &lo, const wide_int &hi, enum tree_code cmp, const REAL_VALUE_TYPE *r, format_helper fmt, - wide_int i, signop isign); + const wide_int &i, signop isign); extern tree combine_comparisons (location_t, enum tree_code, enum tree_code, enum tree_code, tree, tree, tree); @@ -253,8 +253,8 @@ extern tree fold_build_pointer_plus_hwi_loc (location_t loc, tree ptr, HOST_WIDE fold_build_pointer_plus_hwi_loc (UNKNOWN_LOCATION, p, o) extern tree_code minmax_from_comparison (tree_code, tree, - const widest_int, - const widest_int); + const widest_int &, + const widest_int &); extern tree_code minmax_from_comparison (tree_code, tree, tree, tree, tree); diff --git a/gcc/range-op.cc b/gcc/range-op.cc index 350548951f4..3a0e6ed05d1 100644 --- a/gcc/range-op.cc +++ b/gcc/range-op.cc @@ -2585,10 +2585,10 @@ operator_div::wi_fold (irange &r, tree type, const wide_int &lh_lb, const wide_int &lh_ub, const wide_int &rh_lb, const wide_int &rh_ub) const { - const wide_int dividend_min = lh_lb; - const wide_int dividend_max = lh_ub; - const wide_int divisor_min = rh_lb; - const wide_int divisor_max = rh_ub; + const wide_int ÷nd_min = lh_lb; + const wide_int ÷nd_max = lh_ub; + const wide_int &divisor_min = rh_lb; + const wide_int &divisor_max = rh_ub; signop sign = TYPE_SIGN (type); unsigned prec = TYPE_PRECISION (type); wide_int extra_min, extra_max; @@ -4288,7 +4288,7 @@ operator_bitwise_xor::wi_fold (irange &r, tree type, = (wi::bit_and_not (mustbe_nonzero_lh, maybe_nonzero_rh) | wi::bit_and_not (mustbe_nonzero_rh, maybe_nonzero_lh)); wide_int new_ub = ~result_zero_bits; - wide_int new_lb = result_one_bits; + const wide_int &new_lb = result_one_bits; // If the range has all positive or all negative values, the result // is better than VARYING. diff --git a/gcc/tree-ssa-ccp.cc b/gcc/tree-ssa-ccp.cc index cc1da6540e3..1345a065194 100644 --- a/gcc/tree-ssa-ccp.cc +++ b/gcc/tree-ssa-ccp.cc @@ -433,7 +433,8 @@ canonicalize_value (ccp_prop_value_t *val) /* Return whether the lattice transition is valid. */ static bool -valid_lattice_transition (ccp_prop_value_t old_val, ccp_prop_value_t new_val) +valid_lattice_transition (const ccp_prop_value_t &old_val, + const ccp_prop_value_t &new_val) { /* Lattice transitions must always be monotonically increasing in value. */ @@ -1626,7 +1627,7 @@ bit_value_binop (enum tree_code code, signop sgn, int width, is zero. */ if (r2mask == 0) { - widest_int shift = r2val; + const widest_int &shift = r2val; if (shift == 0) { *mask = r1mask; -- 2.53.0
From f34e3c4b816b87abf9f36f9468c5eb0969513e89 Mon Sep 17 00:00:00 2001 From: linden <[email protected]> Date: Sat, 26 Sep 2026 23:34:28 +1200 Subject: [PATCH 8/8] gcov: drop a no-op std::move of a map key In path_info::suppress_blocks, p.first is the key of a std::map element and so is always const; std::move on it still copies. Remove it; behaviour is unchanged. gcc/ChangeLog: * gcov.cc (path_info::suppress_blocks): Remove no-op std::move. Signed-off-by: linden <[email protected]> --- gcc/gcov.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/gcov.cc b/gcc/gcov.cc index 06047a60460..42b483aca87 100644 --- a/gcc/gcov.cc +++ b/gcc/gcov.cc @@ -1121,7 +1121,7 @@ path_info::suppress_blocks (const vector<bool>& suppressed) /* Store the new paths. The map iteration outputs the paths lexicographically ordered. */ for (auto& p : nextpaths) - residual_paths.push_back (std::move (p.first)); + residual_paths.push_back (p.first); } /* Find the arc that connects BLOCK to the block with id DEST, or nullptr if it -- 2.53.0
