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 e9c171d5caedbc1bff3c64e3090958da83e70a92 Mon Sep 17 00:00:00 2001 From: linden <[email protected]> Date: Sat, 26 Sep 2026 18:41:52 +1200 Subject: [PATCH 4/7] gcc, analyzer: drop std::move calls that have no effect Three call sites wrap an argument in std::move where no move can actually happen: the callee only takes a const reference, or there is no std::string::operator+=(std::string&&). Remove the std::move in each; behaviour is unchanged. gcc/ChangeLog: * xml.cc (node_with_children::add_text): Remove no-op std::move. gcc/analyzer/ChangeLog: * access-diagram.cc (access_diagram_impl::make_warning_string): Remove no-op std::move. * engine.cc (exploded_graph::unwind_from_exception): Likewise. Signed-off-by: linden <[email protected]> --- gcc/analyzer/access-diagram.cc | 2 +- gcc/analyzer/engine.cc | 2 +- gcc/xml.cc | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gcc/analyzer/access-diagram.cc b/gcc/analyzer/access-diagram.cc index 9caafcdb55d..861823f6aea 100644 --- a/gcc/analyzer/access-diagram.cc +++ b/gcc/analyzer/access-diagram.cc @@ -2347,7 +2347,7 @@ private: overlapping the next char. Hence we add two spaces here: a space to be covered by this overlap, plus another space of padding. */ result.append (styled_string (m_sm, " ")); - result.append (std::move (text)); + result.append (text); return result; } diff --git a/gcc/analyzer/engine.cc b/gcc/analyzer/engine.cc index 830164dac77..c8a7dcef730 100644 --- a/gcc/analyzer/engine.cc +++ b/gcc/analyzer/engine.cc @@ -1498,7 +1498,7 @@ exploded_graph::unwind_from_exception (exploded_node &thrown_enode, exploded_node *after_unwind_enode = get_or_create_node (unwound_point, - std::move (unwound_state), + unwound_state, iter_enode, /* Don't add this enode to the worklist; we will process it diff --git a/gcc/xml.cc b/gcc/xml.cc index ad6b8460b6f..4659b69afe4 100644 --- a/gcc/xml.cc +++ b/gcc/xml.cc @@ -116,7 +116,7 @@ node_with_children::add_text (std::string str) if (!m_children.empty ()) if (text *t = m_children.back ()->dyn_cast_text ()) { - t->m_str += std::move (str); + t->m_str += str; return; } add_child (std::make_unique <text> (std::move (str))); -- 2.53.0
From 7f9e36e466b376f802616e78d067af698309deb7 Mon Sep 17 00:00:00 2001 From: linden <[email protected]> Date: Sat, 26 Sep 2026 18:41:53 +1200 Subject: [PATCH 6/7] text-art: add array2::set overload taking an rvalue element array2::set only took a const reference, so a std::move at the call site, as in canvas::paint, still copy-assigned. Add an overload that move-assigns, so the std::move takes effect for element types that own a std::vector, such as styled_unichar. gcc/ChangeLog: * text-art/types.h (text_art::array2::set): Add overload taking element_t &&. Signed-off-by: linden <[email protected]> --- gcc/text-art/types.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/gcc/text-art/types.h b/gcc/text-art/types.h index 8778bb1a13d..a3118414612 100644 --- a/gcc/text-art/types.h +++ b/gcc/text-art/types.h @@ -183,6 +183,12 @@ class array2 m_elements[idx] = element; } + void set (const coord_t &coord, element_t &&element) + { + ::size_t idx = get_idx (coord); + m_elements[idx] = std::move (element); + } + void fill (element_t element) { for (int y = 0; y < m_size.h; y++) -- 2.53.0
