Jakub, et al... here is the latest version of the frange endpoints
patch addressing the signed zero problem (well treating +-0.0
ambiguously), as well as implementing all the relational operators.

[Andrew M: I mostly copied our relop code from irange, while keeping
track NANs, etc.  It should be pleasantly familiar.]

A few notes...

We can now represent things like [5.0, 5.0], which is the singleton
5.0 *or* a NAN.  OTOH, we could also have [5.0, 5.0] !NAN, which is
5.0 without the possibility of a NAN.  This allows us to track
appropriate ranges on both sides of an if, but keep track of which
side (true side) we definitely know we won't have a NAN.

As mentioned, frange::singleton_p([0,0]) returns false for any version
of 0.0 (unless !HONOR_SIGNED_ZEROS).  I'll work on zero tracking at a
later time.  The patch is getting pretty large as is.

For convenience, singleton_p() returns false for a NAN.  IMO, it makes
the implementation cleaner, but I'm not wed to the idea if someone
objects.

This patch passes all tests for all languages, including go and ada,
with the aforementioned exception of gcc.dg/tree-ssa/phi-opt-24.c
which is getting confused because we have propagated (correctly) a 0.0
into the PHI.  Hints?  Takers? ;-).

Aldy

On Fri, Aug 26, 2022 at 9:44 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Fri, Aug 26, 2022 at 12:16 PM Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> > On Fri, Aug 26, 2022 at 7:40 PM Andrew Pinski <pinskia@gmail.com> wrote:
> > >
> > > On Fri, Aug 26, 2022 at 8:55 AM Aldy Hernandez <aldyh@redhat.com> wrote:
> > > >
> > > > [pinskia: I'm CCing you as the author of the match.pd pattern.]
> > > >
> > > > So, as I wrap up the work here (latest patch attached), I see there's
> > > > another phiopt regression (not spaceship related).  I was hoping
> > > > someone could either give me a hand, or offer some guidance.
> > > >
> > > > The failure is in gcc.dg/tree-ssa/phi-opt-24.c.
> > > >
> > > > We fail to transform the following into -A:
> > > >
> > > > /* { dg-options "-O2 -fno-signed-zeros -fdump-tree-phiopt" } */
> > > >
> > > > float f0(float A)
> > > > {
> > > >   //     A == 0? A : -A    same as -A
> > > >   if (A == 0)  return A;
> > > >   return -A;
> > > > }
> > > >
> > > > This is because the abs/negative match.pd pattern here:
> > > >
> > > > /* abs/negative simplifications moved from fold_cond_expr_with_comparison,
> > > >    Need to handle (A - B) case as fold_cond_expr_with_comparison does.
> > > >    Need to handle UN* comparisons.
> > > >    ...
> > > >    ...
> > > >
> > > > Sees IL that has the 0.0 propagated.
> > > >
> > > > Instead of:
> > > >
> > > >   <bb 2> [local count: 1073741824]:
> > > >   if (A_2(D) == 0.0)
> > > >     goto <bb 4>; [34.00%]
> > > >   else
> > > >     goto <bb 3>; [66.00%]
> > > >
> > > >   <bb 3> [local count: 708669601]:
> > > >   _3 = -A_2(D);
> > > >
> > > >   <bb 4> [local count: 1073741824]:
> > > >   # _1 = PHI <A_2(D)(2), _3(3)>
> > > >
> > > > It now sees:
> > > >
> > > >   <bb 4> [local count: 1073741824]:
> > > >   # _1 = PHI <0.0(2), _3(3)>
> > > >
> > > > which it leaves untouched, causing the if conditional to survive.
> > > >
> > > > Is this something that can be done by improving the match.pd pattern,
> > > > or should be done elsewhere?
> > >
> > > Oh the pattern which is supposed to catch this does:
> > >   (simplify
> > >    (cnd (cmp @0 zerop) integer_zerop (negate@1 @0))
> > >     (if (!HONOR_SIGNED_ZEROS (type))
> > >      @1))
> >
> > On trunk without any patches, for the following snippet with -O2
> > -fno-signed-zeros -fdump-tree-phiopt-folding...
> >
> > float f0(float A)
> > {
> >   //     A == 0? A : -A    same as -A
> >   if (A == 0)  return A;
> >   return -A;
> > }
> >
> > ...the phiopt2 dump file has:
> >
> > Applying pattern match.pd:4805, gimple-match.cc:69291, which
> > corresponds to the aforementioned pattern.  So it looks like that was
> > the pattern that was matching that isn't any more?
> >
> > Are you saying this pattern should only work with integers?
>
> I am saying the pattern which is right after the one that matches
> (without your patch) currrently works for integer only.
> You could change integer_zerop to zerop in that pattern but I am not
> 100% sure that is valid thing to do.
> Note there are a few other patterns in that for loop that does
> integer_zerop which might need to be zerop too.
>
> Thanks,
> Andrew Pinski
>
> >
> > Aldy
> >
>