Pushed patch to convert DOM from EVRP to Ranger

Pushed patch to convert DOM from EVRP to Ranger

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

This is 99.99% Aldy's work.  My only real contribution was twiddling one 
MIPS specific test in response to a regression after adding this patch 
to my tester.

In simplest terms, this patch converts DOM to use the Ranger 
infrastructure rather than the EVRP infrastructure.  I'd basically 
approved it before Aldy went on PTO and it's been sitting in my tester 
ever since.  So I figured it was time to go ahead and push it.

Jeff

[-- Attachment #2: J --]
[-- Type: text/plain, Size: 6466 bytes --]

commit 8c99e307b20c502e55c425897fb3884ba8f05882
Author: Aldy Hernandez <aldyh@redhat.com>
Date:   Sat Jun 25 18:58:02 2022 -0400

    Convert DOM to use Ranger rather than EVRP
    
    [Jeff, this is the same patch I sent you last week with minor tweaks
    to the commit message.]
    
    [Despite the verbosity of the message, this is actually a pretty
    straightforward patch.  It should've gone in last cycle, but there
    was a nagging regression I couldn't get to until after stage1
    had closed.]
    
    There are 3 uses of EVRP in DOM that must be converted.
    Unfortunately, they need to be converted in one go, so further
    splitting of this patch would be problematic.
    
    There's nothing here earth shattering.  It's all pretty obvious in
    retrospect, but I've added a short description of each use to aid in
    reviewing:
    
    * Convert evrp use in cprop to ranger.
    
      This is easy, as cprop in DOM was converted to the ranger API last
      cycle, so this is just a matter of using a ranger instead of an
      evrp_range_analyzer.
    
    * Convert evrp use in threader to ranger.
    
      The idea here is to use the hybrid approach we used for the initial
      VRP threader conversion last cycle.  The DOM threader will continue
      using the forward threader infrastructure while continuing to query
      DOM data structures, and only if the conditional does not relsolve,
      using the ranger.  This gives us the best of both worlds, and is a
      proven approach.
    
      Furthermore, as frange and prange come live in the next cycle, we
      can move away from the forward threader altogether, and just add
      another backward threader.  This will not only remove the last use
      of the forward threader, but will allow us to remove at least 1 or 2
      threader instances.
    
    * Convert conditional folding to use the method used by the ranger and
      evrp.  Previously DOM was calling into the guts of
      simplify_using_ranges::vrp_visit_cond_stmt.  The blessed way now is
      using fold_cond() which rewrites the conditional and edges
      automatically.
    
      When legacy is removed, simplify_using_ranges will be further
      cleaned up, and there will only be one entry point into simplifying
      a statement.
    
    * DOM was setting global ranges determined from unreachable edges as a
      side-effect of using the evrp engine.  We must handle these cases
      before nuking evrp, and DOM seems like a good fit.  I've just moved
      the snippet to DOM, but it could live anywhere else we do a DOM
      walk.
    
      For the record, this is the case *vrp handled:
    
            <bb C>:
            ...
            if (c_5(D) != 5)
            goto <bb N>;
            else
            goto <bb M>;
            <bb N>:
            __builtin_unreachable ();
            <bb M>:
    
      If M dominates all uses of c_5, we can set the global range of c_5
      to [5,5].
    
    I have tested on x86-64, pcc64le, and aarch64 Linux.
    
    I also ran threading benchmarks as well as performance benchmarks.
    
    DOM threads 1.56% more paths which ultimately yields a miniscule total
    increase of 0.03%.
    
    The conversion to ranger brings a 7.87% performance drop in DOM, which
    is a wash in overall compilation.  This is in line with other
    replacements of legacy evrp with ranger.  We handle a lot more cases.
    It's not free .
    
    There is a a regression on Wstringop-overflow-4.C which I'm planning
    on XFAILing.  It's another variant of the usual middle-end false
    positives: having no ranges produces no warnings, but slightly refined
    ranges, or worse-- isolating specific problematic cases in the
    threader causes flare-ups.
    
    As an aside, as Richi has suggested, I think we should discuss
    restricting the threader's ability to thread highly unlikely paths.
    These cause no end of pain for middle-end warnings.  However,
    I don't know if this would conflict with path isolation for
    things like null dereferencing.  ISTR you were interested in this.
    
    BTW, I think the Wstringop-overflow-4.C test is problematic and I've
    attached my analysis.  Basically the regression is caused by a bad
    interaction with the rounding/alignment that placement new has inlined
    into the IL.  This happens for int16_r[] which the test is testing.
    Ranger can glean some range info, which causes DOM threading to
    isolate a path which causes a warning.
    
    OK for trunk?
    
    gcc/ChangeLog:
    
            * tree-ssa-dom.cc (dom_jt_state): Pass ranger to constructor
            instead of evrp.
            (dom_jt_state::push): Remove m_evrp.
            (dom_jt_state::pop): Same.
            (dom_jt_state::record_ranges_from_stmt): Remove.
            (dom_jt_state::register_equiv): Remove updating of evrp ranges.
            (class dom_jt_simplifier): Pass ranger to constructor.
            Inherit from hybrid_jt_simplifier.
            (dom_jt_simplifier::simplify): Convert to ranger.
            (pass_dominator::execute): Same.
            (all_uses_feed_or_dominated_by_stmt): New.
            (dom_opt_dom_walker::set_global_ranges_from_unreachable_edges): New.
            (dom_opt_dom_walker::before_dom_children): Call
            set_global_ranges_from_unreachable_edges.
            Do not call record_ranges_from_stmt.
            (dom_opt_dom_walker::after_dom_children): Remove evrp use.
            (cprop_operand): Use int_range<> instead of value_range.
            (dom_opt_dom_walker::fold_cond): New.
            (dom_opt_dom_walker::optimize_stmt): Pass ranger to
            cprop_into_stmt.
            Use fold_cond() instead of vrp_visit_cond_stmt().
            * tree-ssa-threadedge.cc (jt_state::register_equivs_stmt): Do not
            pass state to simplifier.
            * vr-values.h (class vr_values): Make fold_cond public.
    
    gcc/testsuite/ChangeLog:
    
            * gcc.dg/sancov/cmp0.c: Adjust for conversion to ranger.
            * gcc.dg/tree-ssa/ssa-dom-branch-1.c: Same.
            * gcc.dg/tree-ssa/ssa-dom-thread-7.c: Same.
            * gcc.dg/vect/bb-slp-pr81635-2.c: Same.
            * gcc.dg/vect/bb-slp-pr81635-4.c: Same.
            * g++.dg/warn/Wstringop-overflow-4.C: Likewise.
            * gcc.target/mips/data-sym-multi-pool.c: Likewise.
            * gcc.target/mips/mips.exp: Likewise.

Re: Pushed patch to convert DOM from EVRP to Ranger

[Aldy Hernandez]

[-- Attachment #1: Type: text/plain, Size: 1056 bytes --]

Thanks for pushing this.

The patch triggered a (known) regression on
g++.dg/warn/Wstringop-overflow-4.C.  In the original submission I
mentioned I would XFAIL it, but forgot to do so.  I have pushed the
attached patch.

Note that since this was the last user of EVRP, I think it is now safe
to remove its code, along with any options on params.def.  Andrew, are
you OK with removing the legacy evrp code (gimple-ssa-evrp-analyze.*,
and any relevant bits)?  Of course, the core VRP code would still
remain, as VRP1 still uses it.

Aldy

On Sun, Jun 26, 2022 at 1:08 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
> This is 99.99% Aldy's work.  My only real contribution was twiddling one
> MIPS specific test in response to a regression after adding this patch
> to my tester.
>
> In simplest terms, this patch converts DOM to use the Ranger
> infrastructure rather than the EVRP infrastructure.  I'd basically
> approved it before Aldy went on PTO and it's been sitting in my tester
> ever since.  So I figured it was time to go ahead and push it.
>
> Jeff

[-- Attachment #2: 0001-XFAIL-a-test-in-g-.dg-warn-Wstringop-overflow-4.C.patch --]
[-- Type: text/x-patch, Size: 1334 bytes --]

From 80ace9185dc534e4d6cb19506300c4fcad4bd916 Mon Sep 17 00:00:00 2001
From: Aldy Hernandez <aldyh@redhat.com>
Date: Sun, 26 Jun 2022 17:30:18 +0200
Subject: [PATCH] XFAIL a test in g++.dg/warn/Wstringop-overflow-4.C

As per the explanation in the test, and in the DOM conversion to
ranger patch, this is a known regression.  I had mentioned I would
XFAIL this test, but forgot to do so.  There is an analysis in the
test itself as to what is going on.

Tested on x86-64 Linux.

gcc/testsuite/ChangeLog:

	* g++.dg/warn/Wstringop-overflow-4.C: XFAIL a test.
---
 gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
index eb4801918fc..c9d63932977 100644
--- a/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
+++ b/gcc/testsuite/g++.dg/warn/Wstringop-overflow-4.C
@@ -195,7 +195,7 @@ void test_strcpy_new_int16_t (size_t n, const size_t vals[])
       iftmp.2_33 = _45 * 2;				;; iftmp.2_33 = 0
       _34 = operator new [] (iftmp.2_33);		;; new [] (0)
   */
-  T (S (2), new int16_t[r_dmin_dmax + 1]);
+  T (S (2), new int16_t[r_dmin_dmax + 1]); // { dg-bogus "into a region of size" "" { xfail *-*-*} }
   T (S (9), new int16_t[r_dmin_dmax * 2 + 1]);
 }
 
-- 
2.36.1

Re: Pushed patch to convert DOM from EVRP to Ranger

[Jeff Law]

On 6/26/2022 9:38 AM, Aldy Hernandez wrote:
> Thanks for pushing this.
>
> The patch triggered a (known) regression on
> g++.dg/warn/Wstringop-overflow-4.C.  In the original submission I
> mentioned I would XFAIL it, but forgot to do so.  I have pushed the
> attached patch.
We both forgot about this :-)

>
> Note that since this was the last user of EVRP, I think it is now safe
> to remove its code, along with any options on params.def.  Andrew, are
> you OK with removing the legacy evrp code (gimple-ssa-evrp-analyze.*,
> and any relevant bits)?  Of course, the core VRP code would still
> remain, as VRP1 still uses it.
Sounds good to me.
jeff

Re: Pushed patch to convert DOM from EVRP to Ranger

[Andrew MacLeod]

On 6/26/22 11:38, Aldy Hernandez wrote:
> Thanks for pushing this.
>
> The patch triggered a (known) regression on
> g++.dg/warn/Wstringop-overflow-4.C.  In the original submission I
> mentioned I would XFAIL it, but forgot to do so.  I have pushed the
> attached patch.
>
> Note that since this was the last user of EVRP, I think it is now safe
> to remove its code, along with any options on params.def.  Andrew, are
> you OK with removing the legacy evrp code (gimple-ssa-evrp-analyze.*,
> and any relevant bits)?  Of course, the core VRP code would still
> remain, as VRP1 still uses it.


I think that would be OK.  We have not turned legacy evrp on for 
anything for some time.  And then any leftover bits that cant be removed 
are probably being invoked from range-ops or somewhere else and we can 
relocate them to where they belong and possibly better consolidate 
them.   At least go thru the excercise and produce the patch and see 
what comes out.  Maybe theres a surprise awaiting you :-)   then we can 
further discuss it.

Im currently  working towards turning ranger on by deafult for VRP1, i 
Just  have a few issues to address.   That'll still probably be a few weeks.

Andfrewe


> Aldy
>
> On Sun, Jun 26, 2022 at 1:08 AM Jeff Law <jeffreyalaw@gmail.com> wrote:
>> This is 99.99% Aldy's work.  My only real contribution was twiddling one
>> MIPS specific test in response to a regression after adding this patch
>> to my tester.
>>
>> In simplest terms, this patch converts DOM to use the Ranger
>> infrastructure rather than the EVRP infrastructure.  I'd basically
>> approved it before Aldy went on PTO and it's been sitting in my tester
>> ever since.  So I figured it was time to go ahead and push it.
>>
>> Jeff