On Thu, 20 Feb 2025, Jeff Law wrote:
>
>
> On 2/20/25 5:47 AM, Richard Biener wrote:
> > When SCCP does final value replacement we end up with unfolded IL like
> >
> > __result_274 = _150 + 1;
> > ...
> > __new_finish_106 = __result_274 + 3; <-- from SCCP
> > _115 = _150 + 4;
> > if (__new_finish_106 != _115)
> >
> > this does only get rectified by the next full folding which happens
> > in forwprop4 which is after the strlen pass emitting the unwanted
> > diagnostic. The following mitigates this case in a similar way as
> > r15-7472 did for PR118817 - by ensuring we have the IL folded.
> > This is done by simply folding all immediate uses of the former
> > PHI def that SCCP replaces. All other more general approaches have
> > too much fallout at this point.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK?
> >
> > Thanks,
> > Richard.
> >
> > PR tree-optimization/118521
> > * tree-scalar-evolution.cc (final_value_replacement_loop):
> > Fold uses of the replaced PHI def.
> >
> > * g++.dg/torture/pr118521.C: New testcase.
> Guess I should have included "have scev clean up its own mess" as an option in
> the BZ :-)
>
> I dismissed pass reordering without even investigating. I'm always skeptical
> of that as the solution.
>
> I'd pondered how to make this work in a DOM context and even tried something
> akin to what you're doing (though just in the debugger and I think I used the
> single use edge callback which obviously didn't work).
>
> Point being I think that's probably the best solution we're going to see.
> LGTM.
>
> I hesitate to even bring it up (but we need to). Do we want to consider the
> state/future of these warnings for gcc-16? Is this stuff salvageable? I
> think there's utility in these warnings, but they're clearly a pain point
> across multiple contexts.
What I'd like to see is less diagnostics emitted from random passes
but instead, like we did with the array-bounds pass, do it separately.
And then have those passes not be randomly spread but in one or two
places.
But in general as you say the "advancd IL diagnostics" face the
same issue as static analyzers - we usually lack knowledge of all
pre-conditions to avoid false positives and there's no nice way
to annotate source with those in a way not affecting code generation
(because we warn off the IL, so those need to be in the IL itself).
The analyzer should have the very same issue here. There's Qings
patch to improve the diagnostic to help users write missing
pre-conditions, so that's a thing to look at.
Other than that - I don't have a good solution.
Richard.
> jeff
>
>
>
>
> > ---
> > gcc/testsuite/g++.dg/torture/pr118521.C | 14 ++++++++++++++
> > gcc/tree-scalar-evolution.cc | 18 ++++++++++++++++++
> > 2 files changed, 32 insertions(+)
> > create mode 100644 gcc/testsuite/g++.dg/torture/pr118521.C
> >
> > diff --git a/gcc/testsuite/g++.dg/torture/pr118521.C
> > b/gcc/testsuite/g++.dg/torture/pr118521.C
> > new file mode 100644
> > index 00000000000..1a66aca0e8d
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/torture/pr118521.C
> > @@ -0,0 +1,14 @@
> > +// { dg-do compile }
> > +// { dg-additional-options "-Wall" }
> > +
> > +#include <vector> // dg-bogus new_allocator.h:191 warning: writing 1 byte
> > into a region of size 0
> > +
> > +void bar(std::vector<char>);
> > +
> > +void foo()
> > +{
> > + std::vector<char> v{1, 2};
> > + v.insert(v.end(), 2, 0);
> > + v.push_back(1);
> > + bar(v);
> > +}
> > diff --git a/gcc/tree-scalar-evolution.cc b/gcc/tree-scalar-evolution.cc
> > index 0ba85917d41..4ca08751a0d 100644
> > --- a/gcc/tree-scalar-evolution.cc
> > +++ b/gcc/tree-scalar-evolution.cc
> > @@ -284,6 +284,7 @@ along with GCC; see the file COPYING3. If not see
> > #include "tree-into-ssa.h"
> > #include "builtins.h"
> > #include "case-cfn-macros.h"
> > +#include "tree-eh.h"
> >
> > static tree analyze_scalar_evolution_1 (class loop *, tree);
> > static tree analyze_scalar_evolution_for_address_of (class loop *loop,
> > @@ -3947,6 +3948,23 @@ final_value_replacement_loop (class loop *loop)
> > print_gimple_stmt (dump_file, SSA_NAME_DEF_STMT (rslt), 0);
> > fprintf (dump_file, "\n");
> > }
> > +
> > + /* Re-fold immediate uses of the replaced def, but avoid
> > + CFG manipulations from this function. For now only do
> > + a single-level re-folding, not re-folding uses of
> > + folded uses. */
> > + if (! SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rslt))
> > + {
> > + gimple *use_stmt;
> > + imm_use_iterator imm_iter;
> > + FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, rslt)
> > + {
> > + gimple_stmt_iterator gsi = gsi_for_stmt (use_stmt);
> > + if (!stmt_can_throw_internal (cfun, use_stmt)
> > + && fold_stmt (&gsi, follow_all_ssa_edges))
> > + update_stmt (gsi_stmt (gsi));
> > + }
> > + }
> > }
> >
> > return any;
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
