Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Sep 4, 2023 at 11:06 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 9/1/23 11:30, Andrew Pinski via Gcc-patches wrote:
> > So it turns out there was a simplier way of starting to
> > improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> > That was rewrite test_for_singularity to use range_op_handler
> > and Value_Range.
> >
> > This patch implements that and
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > gcc/ChangeLog:
> >
> >       * vr-values.cc (test_for_singularity): Add edge argument
> >       and rewrite using range_op_handler.
> >       (simplify_compare_using_range_pairs): Use Value_Range
> >       instead of value_range and update test_for_singularity call.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.dg/tree-ssa/vrp124.c: New test.
> >       * gcc.dg/tree-ssa/vrp125.c: New test.
> > ---
>
> > diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> > index 52ab4fe6109..2474e57ee90 100644
> > --- a/gcc/vr-values.cc
> > +++ b/gcc/vr-values.cc
> > @@ -904,69 +904,33 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> >   }
> >
> >   /* We are comparing trees OP1 and OP2 using COND_CODE.  OP1 has
> > -   a known value range VR.
> > +   a known value range OP1_RANGE.
> >
> >      If there is one and only one value which will satisfy the
> > -   conditional, then return that value.  Else return NULL.
> > -
> > -   If signed overflow must be undefined for the value to satisfy
> > -   the conditional, then set *STRICT_OVERFLOW_P to true.  */
> > +   conditional on the EDGE, then return that value.
> > +   Else return NULL.  */
> >
> >   static tree
> >   test_for_singularity (enum tree_code cond_code, tree op1,
> > -                   tree op2, const value_range *vr)
> > +                   tree op2, const int_range_max &op1_range, bool edge)
> >   {
> > -  tree min = NULL;
> > -  tree max = NULL;
> > -
> > -  /* Extract minimum/maximum values which satisfy the conditional as it was
> > -     written.  */
> > -  if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> > +  /* This is already a singularity.  */
> > +  if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> > +    return NULL;
> I don't think this is necessarily the right thing to do for NE.
>
> Consider if op1 has the range [0,1] and op2 has the value 1.  If the
> code is NE, then we should be able to return a singularity of 0 since
> that's the only value for x where x ne 1 is true given the range for x.

The "false" edge singularity is already known when NE is supplied. I
don't think changing it to the "true" edge singularity will be helpful
all of the time; preferring the value of 0 is a different story.
But that is a different patch and for a different location rather than
inside VRP; it should be in either isel or expand (more likely isel).

Thanks,
Andrew

>
>
>
> I like what you're trying to do, it just needs a bit of refinement I think.
>
> jeff