On Fri, 2026-10-09 at 13:14 +1300, best xmg wrote: > 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.
Hi Linden - thanks for the patches. These remaining patches relate to the middle-end generally rather than the analyzer, so I'm not the best person to review them. I suggest re-sending them, split out into individual mails per patch, with an updated Subject line reflecting what the patch does, so they're more likely to be seen by the appropriate people. Hope this is helpful Dave > > 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 > > > > > > >
