RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Richard Biener]

On Tue, 29 Aug 2017, Tamar Christina wrote:

> 
> 
> > -Original Message-
> > From: Richard Biener [mailto:rguent...@suse.de]
> > Sent: 24 August 2017 10:16
> > To: Tamar Christina
> > Cc: Joseph Myers; Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC
> > Patches; Wilco Dijkstra; Michael Meissner; nd
> > Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> > numbers in GIMPLE.
> > 
> > On Wed, 23 Aug 2017, Tamar Christina wrote:
> > 
> > > Hi Richard,
> > >
> > > Thanks for the feedback,
> > >
> > > >
> > > > I think you should split up your work a bit.  The pieces from
> > > > fold_builtin_* you remove and move to lowering that require no
> > > > control flow like __builtin_isnan (x) -> x UNORD x should move to
> > > > match.pd patterns (aka foldings that will then be applied both on
> > GENERIC and GIMPLE).
> > > >
> > >
> > > But emitting them as an UNORD wouldn't solve the performance or
> > > correctness issues The patch is trying to address. For correctness,
> > > for instance an UNORD would still signal When -fsignaling-nans is used
> > (PR/66462).
> > 
> > Well, then address the correctness issues separately.  I think lumping
> > everything into a single patch just makes your live harder ;)
> 
> But the patch was made to fix the correctness issues. Which unless I'm 
> mistaken can only be solved using integer operations. To give a short 
> history on the patch:
> 
> My original patch had this in builtins.c where the current code is and 
> had none of this problem. The intention of this patch was just address 
> the correctness issues with isnan etc.
> 
> Since we had to do this using integer operations we figured we'd also 
> replace the operations with faster ones so the code was tweaked in order 
> to e.g. make the common paths of fpclassify exit earlier. (I had 
> provided benchmark results to show that the integer operations are 
> faster than the floating point ones, also on x86).
> 
> It was then suggested by upstream review that I do this as a gimple lowering
> phase. I pushed back against this but ultimately lost and submitted a new 
> version
> that moved from builtins.c to gimple-low.c, which was why late expansion for 
> C++
> broke, because the expansion is now done very early.
> 
> The rational for this move was that it would give the rest of the pipeline a 
> chance
> to optimize the code further.
> 
> After a few revisions this was ultimately accepted (the v3 version), but was 
> reverted
> because it broke IBM's long double format due to the special case code for it 
> violating SSA.
> 
> That was trivially fixed (once I finally had access to a PowerPC hardware 
> months later) and I changed the
> patch to leave the builtin as a function call if at lowering time it wasn't 
> known to be a builtin.
> This then "fixes" the C++ testcase, in that the test doesn't fail anymore, 
> but it doesn't generate
> the same code as before.
> Though the test is quite contrived and I don't know if actual user code often 
> has this.
> 
> While checking to see if this behavior is OK, I was suggested to do it 
> as a gimple-fold instead. I did, but then turns out it won't work for 
> non-constants as I can't generate new control flow from a fold (which 
> makes sense in retrospect but I didn't know this before).
> 
> This is why the patch is more complex than what it was before. The 
> original version only emitted simple tree.

The correctness issue is only about -fsignaling-nans, correct?  So a
simple fix is to guard the existing builtins.c code with
!flag_singalling_nans (and emit a library call for -fsignalling-nans).

> > 
> > I was saying that if I write isunordered (x, y) and use -fno-signaling-nans 
> > (the
> > default) then I would expect the same code to be generated as if I say isnan
> > (x) || isnan (y).  And I'm not sure if isnan (x) || isnan (y) is more 
> > efficient if
> > both are implemented with integer ops compared to using an unordered FP
> > compare.
> > 
> 
> But isnan (x) || isnan (y) can be rewritten using a match.pd rule to 
> isunordered (x, y)
> If that should really generate the same code. So I don’t think it's an issue 
> for this patch.

If isnan is lowered to integer ops than we'll have a hard time recognizing
it.

> > A canonical high-level representation on GIMPLE is equally important as
> > maybe getting some optimization on a lowered "optimized" form.
> > 
> > And a good canonical representation is short (x UNORD x or x UNORD y
> >

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

> -Original Message-
> From: Richard Biener [mailto:rguent...@suse.de]
> Sent: 24 August 2017 10:16
> To: Tamar Christina
> Cc: Joseph Myers; Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC
> Patches; Wilco Dijkstra; Michael Meissner; nd
> Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
> 
> On Wed, 23 Aug 2017, Tamar Christina wrote:
> 
> > Hi Richard,
> >
> > Thanks for the feedback,
> >
> > >
> > > I think you should split up your work a bit.  The pieces from
> > > fold_builtin_* you remove and move to lowering that require no
> > > control flow like __builtin_isnan (x) -> x UNORD x should move to
> > > match.pd patterns (aka foldings that will then be applied both on
> GENERIC and GIMPLE).
> > >
> >
> > But emitting them as an UNORD wouldn't solve the performance or
> > correctness issues The patch is trying to address. For correctness,
> > for instance an UNORD would still signal When -fsignaling-nans is used
> (PR/66462).
> 
> Well, then address the correctness issues separately.  I think lumping
> everything into a single patch just makes your live harder ;)

But the patch was made to fix the correctness issues. Which unless I'm mistaken
can only be solved using integer operations. To give a short history on the 
patch:

My original patch had this in builtins.c where the current code is and had
none of this problem. The intention of this patch was just address the 
correctness
issues with isnan etc.

Since we had to do this using integer operations we figured we'd also replace 
the
operations with faster ones so the code was tweaked in order to e.g. make the 
common
paths of fpclassify exit earlier. (I had provided benchmark results to show that
the integer operations are faster than the floating point ones, also on x86).

It was then suggested by upstream review that I do this as a gimple lowering
phase. I pushed back against this but ultimately lost and submitted a new 
version
that moved from builtins.c to gimple-low.c, which was why late expansion for C++
broke, because the expansion is now done very early.

The rational for this move was that it would give the rest of the pipeline a 
chance
to optimize the code further.

After a few revisions this was ultimately accepted (the v3 version), but was 
reverted
because it broke IBM's long double format due to the special case code for it 
violating SSA.

That was trivially fixed (once I finally had access to a PowerPC hardware 
months later) and I changed the
patch to leave the builtin as a function call if at lowering time it wasn't 
known to be a builtin.
This then "fixes" the C++ testcase, in that the test doesn't fail anymore, but 
it doesn't generate
the same code as before.
Though the test is quite contrived and I don't know if actual user code often 
has this.

While checking to see if this behavior is OK, I was suggested to do it as a 
gimple-fold instead.
I did, but then turns out it won't work for non-constants as I can't generate 
new control flow from
a fold (which makes sense in retrospect but I didn't know this before).

This is why the patch is more complex than what it was before. The original 
version only emitted simple tree.

> 
> I was saying that if I write isunordered (x, y) and use -fno-signaling-nans 
> (the
> default) then I would expect the same code to be generated as if I say isnan
> (x) || isnan (y).  And I'm not sure if isnan (x) || isnan (y) is more 
> efficient if
> both are implemented with integer ops compared to using an unordered FP
> compare.
> 

But isnan (x) || isnan (y) can be rewritten using a match.pd rule to 
isunordered (x, y)
If that should really generate the same code. So I don’t think it's an issue 
for this patch.

> A canonical high-level representation on GIMPLE is equally important as
> maybe getting some optimization on a lowered "optimized" form.
> 
> And a good canonical representation is short (x UNORD x or x UNORD y
> counts as short here).
> 
> > > As fallback the expanders should simply emit a call (as done for
> > > isinf when not folded for example).
> >
> > Yes the patch currently already does this.
> 
> Ah, I probably looked at an old version then.
> 
> > My question had more to do if
> > the late expansion when you alias one of these functions in C++ was
> > really an issue, since It used to (sometimes, only when you got the
> > type of the argument correct) expand before whereas now it'll always
> > just emit a function call in this edge case.
> 
> The answer here is really that we should perform lowering at a point where
> we do not omit possible optimizations.  For the particular case this would
> mean lowering after IPA opt

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Richard Biener]

On Wed, 23 Aug 2017, Tamar Christina wrote:

> Hi Richard,
> 
> Thanks for the feedback,
> 
> > 
> > I think you should split up your work a bit.  The pieces from
> > fold_builtin_* you remove and move to lowering that require no control flow
> > like __builtin_isnan (x) -> x UNORD x should move to match.pd patterns (aka
> > foldings that will then be applied both on GENERIC and GIMPLE).
> > 
> 
> But emitting them as an UNORD wouldn't solve the performance or correctness 
> issues
> The patch is trying to address. For correctness, for instance an UNORD would 
> still signal
> When -fsignaling-nans is used (PR/66462).

Well, then address the correctness issues separately.  I think lumping
everything into a single patch just makes your live harder ;)

I was saying that if I write isunordered (x, y) and use 
-fno-signaling-nans
(the default) then I would expect the same code to be generated as if
I say isnan (x) || isnan (y).  And I'm not sure if isnan (x) || isnan (y)
is more efficient if both are implemented with integer ops compared
to using an unordered FP compare.

A canonical high-level representation on GIMPLE is equally important
as maybe getting some optimization on a lowered "optimized" form.

And a good canonical representation is short (x UNORD x or x UNORD y
counts as short here).

> > As fallback the expanders should simply emit a call (as done for isinf when
> > not folded for example).
> 
> Yes the patch currently already does this.

Ah, I probably looked at an old version then.

> My question had more to do if 
> the late expansion when you alias one of these functions in C++ was 
> really an issue, since It used to (sometimes, only when you got the type 
> of the argument correct) expand before whereas now it'll always just 
> emit a function call in this edge case.

The answer here is really that we should perform lowering at a point
where we do not omit possible optimizations.  For the particular case
this would mean lowering after IPA optimizations (also think of
accelerator offloading which uses the early optimization phase of
the host).  This also allows early optimization to more accurately
estimate code size (when optimizing for size).  And I still think
whether to expose fp classification as FP or integer ops should be
a target decision and done late.

This still means canonicalizations like isnan -> UNORD should happen
early and during folding.

Note __builtin_fpclassify is really a special case and lowering that
early is a good thing -- I'd simply keep the current code here at
the start.

> > I think fpclassify is special (double-check), you can't have an indirect 
> > call to
> > the classification family as they are macros (builtins where they exist 
> > could be
> > called indirectly though I guess we should simply disallow taking their
> > address).  These are appropriate for early lowering like you do.  You can 
> > leave
> > high-level ops from fpclassify lowering and rely on folding to turn them 
> > into
> > sth more optimal.
> 
> Fpclassify is also a function in C++, so in that regard it's not 
> different from the rest. For C code my patch will always do the right 
> thing as like you said they're macros so I would always be able to lower 
> them early.

Hum, but fpclassify is then not a builtin in C++ given it needs to know
the actual values of FP_NAN and friends.  That is, we are talking about
__builtin_fpclassify which does not match the standards fpclassify API.
The builtin is always a function and never a macro.

> > 
> > I still don't fully believe in lowering to integer ops like the isnan 
> > lowering you
> > are doing.  That hides what you are doing from (not yet existing) 
> > propagation
> > passes of FP classification.  I'd do those transforms only late.
> > You are for example missing to open-code isunordered with integer ops, no?
> > I'd have isnan become UNORDERED_EXPR and eventually expand that with
> > bitops.
> 
> I'm not sure how I would be able to distinguish between the 
> UNORDERED_EXPR of an isnan call and that or a general X UNORDERED_EXPR X 
> expression. The new isnan code that replaces the UNORD isn't a general 
> comparison, it can only really check for nan. In general don't how if I 
> rewrite these to TREE expressions early in match.pd how I would ever be 
> able to recognize the very specific cases they try to handle in order to 
> do bitops only when it makes sense.

I believe the reverse transform x UNORD x to isnan (x) is also valid
(modulo -fsignalling-nans).

> 
> I think C++ cases will still be problematic, since match.pd will match on
> 
> int (*xx)(...);
> xx = isnan;
> if (xx(a))
> 
> quite late, and only when xx is eventually replaced by isnan, so a lot 
> of optimizations would have already ignored the UNORD it would have 
> generated. Which means the bitops have to be quite late and would miss a 
> lot of other optimization phases.

Is that so?  Did you try whether vectorization likes x UNORD x or
the bit operations variant of 

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi Richard,

Thanks for the feedback,

> 
> I think you should split up your work a bit.  The pieces from
> fold_builtin_* you remove and move to lowering that require no control flow
> like __builtin_isnan (x) -> x UNORD x should move to match.pd patterns (aka
> foldings that will then be applied both on GENERIC and GIMPLE).
> 

But emitting them as an UNORD wouldn't solve the performance or correctness 
issues
The patch is trying to address. For correctness, for instance an UNORD would 
still signal
When -fsignaling-nans is used (PR/66462).

> As fallback the expanders should simply emit a call (as done for isinf when
> not folded for example).

Yes the patch currently already does this. My question had more to do if the 
late expansion
when you alias one of these functions in C++ was really an issue, since It used 
to (sometimes,
only when you got the type of the argument correct) expand before whereas now 
it'll always
just emit a function call in this edge case.

> I think fpclassify is special (double-check), you can't have an indirect call 
> to
> the classification family as they are macros (builtins where they exist could 
> be
> called indirectly though I guess we should simply disallow taking their
> address).  These are appropriate for early lowering like you do.  You can 
> leave
> high-level ops from fpclassify lowering and rely on folding to turn them into
> sth more optimal.

Fpclassify is also a function in C++, so in that regard it's not different from 
the rest.
For C code my patch will always do the right thing as like you said they're 
macros so
I would always be able to lower them early.

> 
> I still don't fully believe in lowering to integer ops like the isnan 
> lowering you
> are doing.  That hides what you are doing from (not yet existing) propagation
> passes of FP classification.  I'd do those transforms only late.
> You are for example missing to open-code isunordered with integer ops, no?
> I'd have isnan become UNORDERED_EXPR and eventually expand that with
> bitops.

I'm not sure how I would be able to distinguish between the UNORDERED_EXPR of
an isnan call and that or a general X UNORDERED_EXPR X expression. The new 
isnan code
that replaces the UNORD isn't a general comparison, it can only really check 
for nan.
In general don't how if I rewrite these to TREE expressions early in match.pd 
how I would
ever be able to recognize the very specific cases they try to handle in order 
to do bitops only
when it makes sense.

I think C++ cases will still be problematic, since match.pd will match on

int (*xx)(...);
xx = isnan;
if (xx(a))

quite late, and only when xx is eventually replaced by isnan, so a lot of 
optimizations would have
already ignored the UNORD it would have generated. Which means the bitops have 
to be quite late
and would miss a lot of other optimization phases.

It's these cases where you do stuff with a function that you can't with a macro 
that's a problem for my
Current patch. It'll just always leave it in as a function call (the current 
expand code will expand to a cmp if
The types end up matching, but only then), the question is really if it's a big 
issue or not.

And if it is, maybe we should do that rewriting early on. It seems like 
replacing xx with isnan early on
Would have lots of benefits like getting type errors. Since for instance this 
is wrong, but gcc still produces code for it:

int g;

extern "C" int isnan ();
struct bar { int b; };

void foo(struct bar a) {
  int (*xx)(...);
  xx = isnan;
  if (xx(a))
g++;
}

And makes a call to isnan with a struct as an argument.
For the record, with my current patch, testsuite/g++.dg/opt/pr60849.C passes 
since it only checks for compile,
But with a call instead of a compare in the generated code.

Thanks,
Tamar

> 
> That said, split out the uncontroversical part of moving existing foldings 
> from
> builtins.c to match.pd where they generate no control-flow.
> 
> Thanks,
> Richard.
> 
> > Originally the patch was in expand, which would have covered the late
> > expansion case similar to what it's doing now in trunk. I was however
> > asked to move this to a GIMPLE lowering pass as to give other
> optimizations a chance to do further optimizations on the generated code.
> >
> > This of course works fine for C since these math functions are a Macro in C
> but are functions in C++.
> >
> > C++ would then allow you to do stuff like take the address of the
> > C++ function so
> >
> > void foo(float a) {
> >   int (*xx)(...);
> >   xx = isnan;
> >   if (xx(a))
> > g++;
> > }
> >
> > is perfectly legal. My current patch leaves "isnan" in as a call as by
> > the time we're doing GIMPLE lowering the alias has not been resolved
> > yet, whereas the version currently committed is able to expand it as it's 
> > late
> in expand.
> >
> > Curiously the behaviour is somewhat confusing.
> > if xx is called with a non-constant it is expanded as you would expect
> >
> > .LFB0:
> > .cfi_startproc
> >   

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Richard Biener]

On Wed, 23 Aug 2017, Tamar Christina wrote:

> Ping
> 
> From: Tamar Christina
> Sent: Thursday, August 17, 2017 11:49:51 AM
> To: Richard Biener; Joseph Myers
> Cc: Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC Patches; Wilco 
> Dijkstra; Michael Meissner; nd
> Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
> in GIMPLE.
> 
> > -Original Message-
> > From: Richard Biener [mailto:rguent...@suse.de]
> > Sent: 08 June 2017 19:23
> > To: Joseph Myers
> > Cc: Tamar Christina; Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC
> > Patches; Wilco Dijkstra; Michael Meissner; nd
> > Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> > numbers in GIMPLE.
> >
> > On June 8, 2017 6:44:01 PM GMT+02:00, Joseph Myers
> > <jos...@codesourcery.com> wrote:
> > >On Thu, 8 Jun 2017, Richard Biener wrote:
> > >
> > >> For a built-in this is generally valid.  For plain isnan it depends
> > >on
> > >> what the standard says.
> > >>
> > >> You have to support taking the address of isnan anyway and thus
> > >> expanding to a library call in that case.  Why doesn't that not work?
> > >
> > >In the case of isnan there is the Unix98 non-type-generic function, so
> > >it should definitely work to take the address of that function as
> > >declared in the system headers.
> > >
> > >For the DEF_GCC_BUILTIN type-generic functions there may not be any
> > >corresponding library function at all (as well as only being callable
> > >with the __builtin_* name).
> >
> > I haven't followed the patches in detail but I would suggest to move the
> > lowering somewhere to gimple-fold.c so that late discovered direct calls are
> > also lowered.
> 
> I can't do it in fold as in the case where the input isn't a constant it 
> can't be folder away and I would have to introduce new code. Because of 
> how often folds are called it seems to be invalid at certain points to 
> introduce new control flow.

Yes, new control flow shouldn't be emitted from foldings.

> So while I have fixed the ICEs for PowerPC, AIX and the rest I'm still 
> not sure what to do about the late expansion behaviour change.

I think you should split up your work a bit.  The pieces from
fold_builtin_* you remove and move to lowering that require no
control flow like __builtin_isnan (x) -> x UNORD x should move
to match.pd patterns (aka foldings that will then be applied
both on GENERIC and GIMPLE).

As fallback the expanders should simply emit a call (as done
for isinf when not folded for example).

I think fpclassify is special (double-check), you can't have an indirect 
call to the
classification family as they are macros (builtins where they
exist could be called indirectly though I guess we should simply
disallow taking their address).  These are appropriate for
early lowering like you do.  You can leave high-level ops
from fpclassify lowering and rely on folding to turn them
into sth more optimal.

I still don't fully believe in lowering to integer ops
like the isnan lowering you are doing.  That hides what you
are doing from (not yet existing) propagation passes of
FP classification.  I'd do those transforms only late.
You are for example missing to open-code isunordered with
integer ops, no?  I'd have isnan become UNORDERED_EXPR and
eventually expand that with bitops.

That said, split out the uncontroversical part of moving existing
foldings from builtins.c to match.pd where they generate no
control-flow.

Thanks,
Richard.

> Originally the patch was in expand, which would have covered the late 
> expansion case similar to what it's
> doing now in trunk. I was however asked to move this to a GIMPLE lowering 
> pass as to give other optimizations
> a chance to do further optimizations on the generated code.
> 
> This of course works fine for C since these math functions are a Macro in C 
> but are functions in C++.
> 
> C++ would then allow you to do stuff like take the address of the function so
> 
> void foo(float a) {
>   int (*xx)(...);
>   xx = isnan;
>   if (xx(a))
> g++;
> }
> 
> is perfectly legal. My current patch leaves "isnan" in as a call as by the 
> time we're doing GIMPLE lowering
> the alias has not been resolved yet, whereas the version currently committed 
> is able to expand it as it's late
> in expand.
> 
> Curiously the behaviour is somewhat confusing.
> if xx is called with a non-constant it is expanded as you would expect
> 
> .LFB0:
> .cfi_startproc
> fcvtd0, s0
> fcmpd0, d0
> bvs .L

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Ping

From: Tamar Christina
Sent: Thursday, August 17, 2017 11:49:51 AM
To: Richard Biener; Joseph Myers
Cc: Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC Patches; Wilco 
Dijkstra; Michael Meissner; nd
Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

> -Original Message-
> From: Richard Biener [mailto:rguent...@suse.de]
> Sent: 08 June 2017 19:23
> To: Joseph Myers
> Cc: Tamar Christina; Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC
> Patches; Wilco Dijkstra; Michael Meissner; nd
> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
>
> On June 8, 2017 6:44:01 PM GMT+02:00, Joseph Myers
> <jos...@codesourcery.com> wrote:
> >On Thu, 8 Jun 2017, Richard Biener wrote:
> >
> >> For a built-in this is generally valid.  For plain isnan it depends
> >on
> >> what the standard says.
> >>
> >> You have to support taking the address of isnan anyway and thus
> >> expanding to a library call in that case.  Why doesn't that not work?
> >
> >In the case of isnan there is the Unix98 non-type-generic function, so
> >it should definitely work to take the address of that function as
> >declared in the system headers.
> >
> >For the DEF_GCC_BUILTIN type-generic functions there may not be any
> >corresponding library function at all (as well as only being callable
> >with the __builtin_* name).
>
> I haven't followed the patches in detail but I would suggest to move the
> lowering somewhere to gimple-fold.c so that late discovered direct calls are
> also lowered.

I can't do it in fold as in the case where the input isn't a constant it can't 
be folder away
and I would have to introduce new code. Because of how often folds are called 
it seems to be invalid
at certain points to introduce new control flow.

So while I have fixed the ICEs for PowerPC, AIX and the rest I'm still not sure 
what to do about the
late expansion behaviour change.

Originally the patch was in expand, which would have covered the late expansion 
case similar to what it's
doing now in trunk. I was however asked to move this to a GIMPLE lowering pass 
as to give other optimizations
a chance to do further optimizations on the generated code.

This of course works fine for C since these math functions are a Macro in C but 
are functions in C++.

C++ would then allow you to do stuff like take the address of the function so

void foo(float a) {
  int (*xx)(...);
  xx = isnan;
  if (xx(a))
g++;
}

is perfectly legal. My current patch leaves "isnan" in as a call as by the time 
we're doing GIMPLE lowering
the alias has not been resolved yet, whereas the version currently committed is 
able to expand it as it's late
in expand.

Curiously the behaviour is somewhat confusing.
if xx is called with a non-constant it is expanded as you would expect

.LFB0:
.cfi_startproc
fcvtd0, s0
fcmpd0, d0
bvs .L4

but when xx is called with a constant, say 0 it's not

.LFB0:
.cfi_startproc
mov w0, 0
bl  isnan
cbz w0, .L1

because it infers the type of the 0 to be an integer and then doesn't recognize 
the call.
using 0.0 works, but the behaviour is really counter intuitive.

The question is what should I do now, clearly it would be useful to handle the 
late expansion as well,
however I don't know what the best approach would be.

I can either:

1) Add two new implementations, one for constant folding and one for expansion, 
but then one in expand would
   be quite similar to the one in the early lowering phase. The constant 
folding one could be very simple since
   it's a constant I can just call the buildins and evaluate the value 
completely.

2) Use the patch as is but make another one to allow the renaming to be applied 
quite early on. e.g while still in Tree or GIMPLE resolve

int (*xx)(...);
xx = isnan;
if (xx(a))

to

int (*xx)(...);
xx = isnan;
if (isnan(a))

   This seems like it would be the best approach and the more useful one in 
general.

Any thoughts?

Thanks,
Tamar

>
> Richard.

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

> -Original Message-
> From: Richard Biener [mailto:rguent...@suse.de]
> Sent: 08 June 2017 19:23
> To: Joseph Myers
> Cc: Tamar Christina; Christophe Lyon; Markus Trippelsdorf; Jeff Law; GCC
> Patches; Wilco Dijkstra; Michael Meissner; nd
> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
> 
> On June 8, 2017 6:44:01 PM GMT+02:00, Joseph Myers
> <jos...@codesourcery.com> wrote:
> >On Thu, 8 Jun 2017, Richard Biener wrote:
> >
> >> For a built-in this is generally valid.  For plain isnan it depends
> >on
> >> what the standard says.
> >>
> >> You have to support taking the address of isnan anyway and thus
> >> expanding to a library call in that case.  Why doesn't that not work?
> >
> >In the case of isnan there is the Unix98 non-type-generic function, so
> >it should definitely work to take the address of that function as
> >declared in the system headers.
> >
> >For the DEF_GCC_BUILTIN type-generic functions there may not be any
> >corresponding library function at all (as well as only being callable
> >with the __builtin_* name).
> 
> I haven't followed the patches in detail but I would suggest to move the
> lowering somewhere to gimple-fold.c so that late discovered direct calls are
> also lowered.

I can't do it in fold as in the case where the input isn't a constant it can't 
be folder away
and I would have to introduce new code. Because of how often folds are called 
it seems to be invalid
at certain points to introduce new control flow.

So while I have fixed the ICEs for PowerPC, AIX and the rest I'm still not sure 
what to do about the
late expansion behaviour change.

Originally the patch was in expand, which would have covered the late expansion 
case similar to what it's
doing now in trunk. I was however asked to move this to a GIMPLE lowering pass 
as to give other optimizations
a chance to do further optimizations on the generated code.

This of course works fine for C since these math functions are a Macro in C but 
are functions in C++.

C++ would then allow you to do stuff like take the address of the function so

void foo(float a) {
  int (*xx)(...);
  xx = isnan;
  if (xx(a))
g++;
}

is perfectly legal. My current patch leaves "isnan" in as a call as by the time 
we're doing GIMPLE lowering
the alias has not been resolved yet, whereas the version currently committed is 
able to expand it as it's late
in expand.

Curiously the behaviour is somewhat confusing.
if xx is called with a non-constant it is expanded as you would expect

.LFB0:
.cfi_startproc
fcvtd0, s0
fcmpd0, d0
bvs .L4

but when xx is called with a constant, say 0 it's not

.LFB0:
.cfi_startproc
mov w0, 0
bl  isnan
cbz w0, .L1

because it infers the type of the 0 to be an integer and then doesn't recognize 
the call.
using 0.0 works, but the behaviour is really counter intuitive.

The question is what should I do now, clearly it would be useful to handle the 
late expansion as well,
however I don't know what the best approach would be.

I can either:

1) Add two new implementations, one for constant folding and one for expansion, 
but then one in expand would
   be quite similar to the one in the early lowering phase. The constant 
folding one could be very simple since
   it's a constant I can just call the buildins and evaluate the value 
completely.
   
2) Use the patch as is but make another one to allow the renaming to be applied 
quite early on. e.g while still in Tree or GIMPLE resolve

int (*xx)(...);
xx = isnan;
if (xx(a))

to

int (*xx)(...);
xx = isnan;
if (isnan(a))
  
   This seems like it would be the best approach and the more useful one in 
general.
   
Any thoughts?

Thanks,
Tamar

> 
> Richard.

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Rainer Orth]

Hi Tamar,

> I have reverted the patch in commit r 249050 until I can fix the late 
> expansion
> and IBM long double issues.

thanks.  Just for the record, Solaris/SPARC (also big-endian) was
affected as well:

https://gcc.gnu.org/ml/gcc-testresults/2017-06/msg00908.html

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

> For a built-in this is generally valid.  For plain isnan it depends on what 
> the
> standard says.
> 
> You have to support taking the address of isnan anyway and thus expanding
> to a library call in that case.  Why doesn't that not work?

Only because I had put a failsafe in builtins.c with a gcc_unreachable () as I 
never expect
The built-ins not to be expanded. The ICEs are coming from this call.

> 
> Richard.
> 
> 
> >
> >From: Tamar Christina
> >Sent: Thursday, June 8, 2017 1:21:44 PM
> >To: Christophe Lyon; Markus Trippelsdorf
> >Cc: Joseph Myers; Jeff Law; GCC Patches; Wilco Dijkstra;
> >rguent...@suse.de; Michael Meissner; nd
> >Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> >numbers in GIMPLE.
> >
> >Thanks, I'm looking at the failure.
> >My final validate seems to have only run the GCC tests.
> >
> >> -Original Message-
> >> From: Christophe Lyon [mailto:christophe.l...@linaro.org]
> >> Sent: 08 June 2017 13:00
> >> To: Markus Trippelsdorf
> >> Cc: Joseph Myers; Tamar Christina; Jeff Law; GCC Patches; Wilco
> >Dijkstra;
> >> rguent...@suse.de; Michael Meissner; nd
> >> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> >> numbers in GIMPLE.
> >>
> >> On 8 June 2017 at 12:30, Markus Trippelsdorf <mar...@trippelsdorf.de>
> >> wrote:
> >> > On 2017.01.19 at 18:20 +, Joseph Myers wrote:
> >> >> On Thu, 19 Jan 2017, Tamar Christina wrote:
> >> >>
> >> >> > Hi Joseph,
> >> >> >
> >> >> > I made the requested changes and did a quick pass over the rest
> >of
> >> >> > the fp cases.
> >> >>
> >> >> I've no further comments, but watch out for any related test
> >failures
> >> >> being reported.
> >> >
> >> > g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.
> >> >
> >>
> >> Same on arm/aarch64, but there are also other regressions on
> >big-endian
> >> configs:
> >> See http://people.linaro.org/~christophe.lyon/cross-
> >> validation/gcc/trunk/249005/report-build-info.html
> >>
> >>
> >> > --
> >> > Markus

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi All,

I have reverted the patch in commit r 249050 until I can fix the late expansion
and IBM long double issues.

Thanks for the feedback,
Tamar

> -Original Message-
> From: Joseph Myers [mailto:jos...@codesourcery.com]
> Sent: 09 June 2017 06:19
> To: David Edelsohn
> Cc: Tamar Christina; Christophe Lyon; Markus Trippelsdorf; Jeffrey Law; Wilco
> Dijkstra; Michael Meissner; nd; GCC Patches
> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
> 
> On Thu, 8 Jun 2017, David Edelsohn wrote:
> 
> > Tamar,
> >
> > This patch has caused a large number of new failures on AIX, including
> > compiler ICEs.
> 
> It also caused ICEs building glibc for all powerpc configurations (i.e.,
> configurations using IBM long double).
> 
> https://sourceware.org/ml/libc-testresults/2017-q2/msg00318.html
> 
> --
> Joseph S. Myers
> jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 8 Jun 2017, David Edelsohn wrote:

> Tamar,
> 
> This patch has caused a large number of new failures on AIX, including
> compiler ICEs.

It also caused ICEs building glibc for all powerpc configurations (i.e., 
configurations using IBM long double).

https://sourceware.org/ml/libc-testresults/2017-q2/msg00318.html

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[David Edelsohn]

Tamar,

This patch has caused a large number of new failures on AIX, including
compiler ICEs.

Thanks, David

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Richard Biener]

On June 8, 2017 6:44:01 PM GMT+02:00, Joseph Myers  
wrote:
>On Thu, 8 Jun 2017, Richard Biener wrote:
>
>> For a built-in this is generally valid.  For plain isnan it depends
>on 
>> what the standard says.
>> 
>> You have to support taking the address of isnan anyway and thus 
>> expanding to a library call in that case.  Why doesn't that not work?
>
>In the case of isnan there is the Unix98 non-type-generic function, so
>it 
>should definitely work to take the address of that function as declared
>in 
>the system headers.
>
>For the DEF_GCC_BUILTIN type-generic functions there may not be any 
>corresponding library function at all (as well as only being callable
>with 
>the __builtin_* name).

I haven't followed the patches in detail but I would suggest to move the 
lowering somewhere to gimple-fold.c so that late discovered direct calls are 
also lowered.

Richard.

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 8 Jun 2017, Richard Biener wrote:

> For a built-in this is generally valid.  For plain isnan it depends on 
> what the standard says.
> 
> You have to support taking the address of isnan anyway and thus 
> expanding to a library call in that case.  Why doesn't that not work?

In the case of isnan there is the Unix98 non-type-generic function, so it 
should definitely work to take the address of that function as declared in 
the system headers.

For the DEF_GCC_BUILTIN type-generic functions there may not be any 
corresponding library function at all (as well as only being callable with 
the __builtin_* name).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Richard Biener]

On June 8, 2017 5:40:06 PM GMT+02:00, Tamar Christina <tamar.christ...@arm.com> 
wrote:
>The testcase does something unexpected
>
>extern "C" int isnan ();
>
>void foo(float a) {
>  int (*xx)(...);
>  xx = isnan;
>  if (xx(a))
>g++;
>}
>
>and I'm wondering if this is a valid thing to do with a builtin. The
>issue is that at the point where gimple lowering is done xx hasn't been
>resolved to isnan yet.
>So it never recognizes the alias. Previously these builtins were being
>resolved in expand, which happens late enough that it has replaced xx
>with isnan.
>
>I can obviously fix the ICE by having the expand code leave the call as
>a call instead of a builtin. But if this is a valid thing for a builtin
>i'm not sure how to
>best resolve this case.

For a built-in this is generally valid.  For plain isnan it depends on what the 
standard says.

You have to support taking the address of isnan anyway and thus expanding to a 
library call in that case.  Why doesn't that not work?

Richard.


>
>From: Tamar Christina
>Sent: Thursday, June 8, 2017 1:21:44 PM
>To: Christophe Lyon; Markus Trippelsdorf
>Cc: Joseph Myers; Jeff Law; GCC Patches; Wilco Dijkstra;
>rguent...@suse.de; Michael Meissner; nd
>Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
>numbers in GIMPLE.
>
>Thanks, I'm looking at the failure.
>My final validate seems to have only run the GCC tests.
>
>> -Original Message-
>> From: Christophe Lyon [mailto:christophe.l...@linaro.org]
>> Sent: 08 June 2017 13:00
>> To: Markus Trippelsdorf
>> Cc: Joseph Myers; Tamar Christina; Jeff Law; GCC Patches; Wilco
>Dijkstra;
>> rguent...@suse.de; Michael Meissner; nd
>> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
>> numbers in GIMPLE.
>>
>> On 8 June 2017 at 12:30, Markus Trippelsdorf <mar...@trippelsdorf.de>
>> wrote:
>> > On 2017.01.19 at 18:20 +, Joseph Myers wrote:
>> >> On Thu, 19 Jan 2017, Tamar Christina wrote:
>> >>
>> >> > Hi Joseph,
>> >> >
>> >> > I made the requested changes and did a quick pass over the rest
>of
>> >> > the fp cases.
>> >>
>> >> I've no further comments, but watch out for any related test
>failures
>> >> being reported.
>> >
>> > g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.
>> >
>>
>> Same on arm/aarch64, but there are also other regressions on
>big-endian
>> configs:
>> See http://people.linaro.org/~christophe.lyon/cross-
>> validation/gcc/trunk/249005/report-build-info.html
>>
>>
>> > --
>> > Markus

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

The testcase does something unexpected

extern "C" int isnan ();

void foo(float a) {
  int (*xx)(...);
  xx = isnan;
  if (xx(a))
g++;
}

and I'm wondering if this is a valid thing to do with a builtin. The issue is 
that at the point where gimple lowering is done xx hasn't been resolved to 
isnan yet.
So it never recognizes the alias. Previously these builtins were being resolved 
in expand, which happens late enough that it has replaced xx with isnan.

I can obviously fix the ICE by having the expand code leave the call as a call 
instead of a builtin. But if this is a valid thing for a builtin i'm not sure 
how to
best resolve this case.

From: Tamar Christina
Sent: Thursday, June 8, 2017 1:21:44 PM
To: Christophe Lyon; Markus Trippelsdorf
Cc: Joseph Myers; Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; 
Michael Meissner; nd
Subject: RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Thanks, I'm looking at the failure.
My final validate seems to have only run the GCC tests.

> -Original Message-
> From: Christophe Lyon [mailto:christophe.l...@linaro.org]
> Sent: 08 June 2017 13:00
> To: Markus Trippelsdorf
> Cc: Joseph Myers; Tamar Christina; Jeff Law; GCC Patches; Wilco Dijkstra;
> rguent...@suse.de; Michael Meissner; nd
> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
>
> On 8 June 2017 at 12:30, Markus Trippelsdorf <mar...@trippelsdorf.de>
> wrote:
> > On 2017.01.19 at 18:20 +, Joseph Myers wrote:
> >> On Thu, 19 Jan 2017, Tamar Christina wrote:
> >>
> >> > Hi Joseph,
> >> >
> >> > I made the requested changes and did a quick pass over the rest of
> >> > the fp cases.
> >>
> >> I've no further comments, but watch out for any related test failures
> >> being reported.
> >
> > g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.
> >
>
> Same on arm/aarch64, but there are also other regressions on big-endian
> configs:
> See http://people.linaro.org/~christophe.lyon/cross-
> validation/gcc/trunk/249005/report-build-info.html
>
>
> > --
> > Markus

RE: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Thanks, I'm looking at the failure.
My final validate seems to have only run the GCC tests.

> -Original Message-
> From: Christophe Lyon [mailto:christophe.l...@linaro.org]
> Sent: 08 June 2017 13:00
> To: Markus Trippelsdorf
> Cc: Joseph Myers; Tamar Christina; Jeff Law; GCC Patches; Wilco Dijkstra;
> rguent...@suse.de; Michael Meissner; nd
> Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like
> numbers in GIMPLE.
> 
> On 8 June 2017 at 12:30, Markus Trippelsdorf <mar...@trippelsdorf.de>
> wrote:
> > On 2017.01.19 at 18:20 +, Joseph Myers wrote:
> >> On Thu, 19 Jan 2017, Tamar Christina wrote:
> >>
> >> > Hi Joseph,
> >> >
> >> > I made the requested changes and did a quick pass over the rest of
> >> > the fp cases.
> >>
> >> I've no further comments, but watch out for any related test failures
> >> being reported.
> >
> > g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.
> >
> 
> Same on arm/aarch64, but there are also other regressions on big-endian
> configs:
> See http://people.linaro.org/~christophe.lyon/cross-
> validation/gcc/trunk/249005/report-build-info.html
> 
> 
> > --
> > Markus

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Christophe Lyon]

On 8 June 2017 at 12:30, Markus Trippelsdorf  wrote:
> On 2017.01.19 at 18:20 +, Joseph Myers wrote:
>> On Thu, 19 Jan 2017, Tamar Christina wrote:
>>
>> > Hi Joseph,
>> >
>> > I made the requested changes and did a quick pass over the rest
>> > of the fp cases.
>>
>> I've no further comments, but watch out for any related test failures
>> being reported.
>
> g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.
>

Same on arm/aarch64, but there are also other regressions on big-endian configs:
See 
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/249005/report-build-info.html


> --
> Markus

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Markus Trippelsdorf]

On 2017.01.19 at 18:20 +, Joseph Myers wrote:
> On Thu, 19 Jan 2017, Tamar Christina wrote:
> 
> > Hi Joseph,
> > 
> > I made the requested changes and did a quick pass over the rest
> > of the fp cases.
> 
> I've no further comments, but watch out for any related test failures 
> being reported.

g++.dg/opt/pr60849.C started ICEing on both X86_64 and ppc64le.

-- 
Markus

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Resending the message to list, Sorry I hadn't noticed somewhere along the line 
it was dropped..

Hi All,

I believe Joseph had no more comments for the patch.

Any other comments or OK for trunk?

Regards,
Tamar

From: Tamar Christina
Sent: Tuesday, May 2, 2017 10:09 AM
To: Bernhard Reutner-Fischer; Joseph Myers
Cc: Jeff Law; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Ping.

Hi All,

Since this didn't manage to get in for GCC 7, I'm looking for approval to 
commit to trunk.

Cheers,
Tamar

From: Tamar Christina
Sent: Thursday, February 9, 2017 1:56:30 PM
To: Bernhard Reutner-Fischer; Joseph Myers
Cc: Jeff Law; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Ping. If not for now can I get approval for when stage1 opens again?

Cheers,
Tamar

From: Tamar Christina
Sent: Monday, January 30, 2017 10:01:33 AM
To: Bernhard Reutner-Fischer; Joseph Myers
Cc: Jeff Law; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Ping?

You initially approved the patch Jeff but there has been some minor changes 
since then.

Regards,
Tamar


From: Tamar Christina
Sent: Wednesday, January 25, 2017 9:38:57 AM
To: Bernhard Reutner-Fischer; Joseph Myers
Cc: Jeff Law; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Thanks, updated.

Aside from the changelog, Ok for trunk?

Thanks,
Tamar


From: Bernhard Reutner-Fischer <rep.dot@gmail.com>
Sent: Tuesday, January 24, 2017 3:43:54 PM
To: Tamar Christina; Joseph Myers
Cc: Jeff Law; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On 23 January 2017 16:32:20 CET, Tamar Christina <tamar.christ...@arm.com> 
wrote:

>gcc/
>2017-01-23  Tamar Christina  <tamar.christ...@arm.com>
>
>   PR middle-end/77925
>   PR middle-end/77926
>   PR middle-end/66462
>
>   * gcc/builtins.c (fold_builtin_fpclassify): Removed.
>   (fold_builtin_interclass_mathfn): Removed.
>   (expand_builtin): Added builtins to lowering list.
>   (fold_builtin_n): Removed fold_builtin_varargs.

Present tense in ChangeLog please.

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Mon, 23 Jan 2017, Tamar Christina wrote:

> In the testcase I've also had to remove `-funsafe-math-optimizations` 
> because at least on AArch64 this sets the LZ bit in the FPCR which 
> causes the fcmp to collapse small subnormal numbers to 0 and so iszero 
> would fail.

That's not an appropriate change to the test; it's clear that test is 
deliberately about testing what happens with -funsafe-math-optimizations.  
Rather, any particular new pieces that won't work with 
-funsafe-math-optimizations should have #ifdef UNSAFE conditionals in 
tg-tests.h, like the existing conditionals there.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi Jeff, Joseph,

Thank you for the reviews. This is hopefully the final version of the patch.

I was doing some extra testing on the FP fallback case
and noticed that the builtins Joseph told me about before were emitting GENERIC
and not GIMPLE code, particularly the TRUTH_NOT_EXPR expression was problematic.
So I've added calls to gimplify_expr and gimple_boolify where appropriate to do
the conversion.

In the testcase I've also had to remove `-funsafe-math-optimizations` because at
least on AArch64 this sets the LZ bit in the FPCR which causes the fcmp to
collapse small subnormal numbers to 0 and so iszero would fail.

With this both the FP and Integer versions have no regressions.

Ok for trunk?

Thanks,

Tamar.

gcc/
2017-01-23  Tamar Christina  <tamar.christ...@arm.com>

PR middle-end/77925
PR middle-end/77926
PR middle-end/66462

* gcc/builtins.c (fold_builtin_fpclassify): Removed.
(fold_builtin_interclass_mathfn): Removed.
(expand_builtin): Added builtins to lowering list.
(fold_builtin_n): Removed fold_builtin_varargs.
(fold_builtin_varargs): Removed.
* gcc/builtins.def (BUILT_IN_ISZERO, BUILT_IN_ISSUBNORMAL): Added.
* gcc/real.h (get_min_float): Added.
(real_format): Added is_ieee_compatible field.
* gcc/real.c (get_min_float): Added.
(ieee_single_format): Set is_ieee_compatible flag.
* gcc/gimple-low.c (lower_stm): Define BUILT_IN_FPCLASSIFY,
CASE_FLT_FN (BUILT_IN_ISINF), BUILT_IN_ISINFD32, BUILT_IN_ISINFD64,
BUILT_IN_ISINFD128, BUILT_IN_ISNAND32, BUILT_IN_ISNAND64,
BUILT_IN_ISNAND128, BUILT_IN_ISNAN, BUILT_IN_ISNORMAL, BUILT_IN_ISZERO,
BUILT_IN_ISSUBNORMAL, CASE_FLT_FN (BUILT_IN_FINITE), BUILT_IN_FINITED32
BUILT_IN_FINITED64, BUILT_IN_FINITED128, BUILT_IN_ISFINITE.
(lower_builtin_fpclassify, is_nan, is_normal, is_infinity): Added.
(is_zero, is_subnormal, is_finite, use_ieee_int_mode): Likewise.
(lower_builtin_isnan, lower_builtin_isinfinite): Likewise.
(lower_builtin_isnormal, lower_builtin_iszero): Likewise.
(lower_builtin_issubnormal, lower_builtin_isfinite): Likewise.
(emit_tree_cond, get_num_as_int, emit_tree_and_return_var): Added.
(mips_single_format): Likewise.
(motorola_single_format): Likewise.
(spu_single_format): Likewise.
(ieee_double_format): Likewise.
(mips_double_format): Likewise.
(motorola_double_format): Likewise.
(ieee_extended_motorola_format): Likewise.
(ieee_extended_intel_128_format): Likewise.
(ieee_extended_intel_96_round_53_format): Likewise.
(ibm_extended_format): Likewise.
(mips_extended_format): Likewise.
(ieee_quad_format): Likewise.
(mips_quad_format): Likewise.
(vax_f_format): Likewise.
(vax_d_format): Likewise.
(vax_g_format): Likewise.
(decimal_single_format): Likewise.
(decimal_quad_format): Likewise.
(iee_half_format): Likewise.
(mips_single_format): Likewise.
(arm_half_format): Likewise.
(real_internal_format): Likewise.
* gcc/doc/extend.texi: Added documentation for built-ins.
* gcc/c/c-typeck.c (convert_arguments): Added BUILT_IN_ISZERO
and BUILT_IN_ISSUBNORMAL.

gcc/testsuite/
2017-01-23  Tamar Christina  <tamar.christ...@arm.com>

* gcc.target/aarch64/builtin-fpclassify.c: New codegen test.
* gcc.dg/fold-notunord.c: Removed.
* gcc.dg/torture/floatn-tg-4.h: Added tests for iszero and issubnormal.
* gcc.dg/torture/float128-tg-4.c: Likewise.
* gcc.dg/torture/float128x-tg-4: Likewise.
* gcc.dg/torture/float16-tg-4.c: Likewise.
* gcc.dg/torture/float32-tg-4.c: Likewise.
* gcc.dg/torture/float32x-tg-4.c: Likewise.
* gcc.dg/torture/float64-tg-4.c: Likewise.
* gcc.dg/torture/float64x-tg-4.c: Likewise.
* gcc.dg/pr28796-1.c: Added -O2.
* gcc.dg/builtins-43.c: Check lower instead of gimple.
* gcc.dg/tg-tests.h: Added iszero and issubnormal.
* gcc.dg/pr28796-2.c: Dropped -funsafe-math-optimizations.

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, January 19, 2017 6:20:35 PM
To: Tamar Christina
Cc: Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; 
nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 19 Jan 2017, Tamar Christina wrote:

> Hi Joseph,
>
> I made the requested changes and did a quick pass over the rest
> of the fp cases.

I've no further comments, but watch out for any related test failures
being reported.

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..d8ff9c70ae6b9e72e09b8cbd9a0bd41b6830b83e 100644

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 19 Jan 2017, Tamar Christina wrote:

> Hi Joseph,
> 
> I made the requested changes and did a quick pass over the rest
> of the fp cases.

I've no further comments, but watch out for any related test failures 
being reported.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi Joseph,

I made the requested changes and did a quick pass over the rest
of the fp cases.

Regards,
Tamar


From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, January 19, 2017 4:33:13 PM
To: Tamar Christina
Cc: Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; 
nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 19 Jan 2017, Tamar Christina wrote:

> > The old code set orig_arg before converting IBM long double to double.
> > Your code sets it after the conversion.  The old code set min_exp based on
> > a string set from REAL_MODE_FORMAT (orig_mode)->emin - 1; your code uses
> > the adjusted mode.  Both of those are incorrect for IBM long double.
>
> Hmm, this is correct, I've made the change to be like it was before, but
> there's something I don't quite get about the old code, if it's building rmin
> the orig_mode which is larger than mode, but then creating the real using
> build_real (type, rmin) using the adjusted type, shouldn't it not be getting
> truncated?

The value is a power of 2, which is *larger* than the minimum normal value
for double (as they have the same least subnormal value).

Looking further at the code, my only remaining comments are for the cases
where you aren't using the integer path: in is_normal you use LT_EXPR to
compare with +Inf, but need to use __builtin_isless, likewise in
is_subnormal again you should be using __builtin_isless and
__builtin_isgreater, in is_finite you should be using
__builtin_islessequal.  All the existing expressions will raise spurious
"invalid" exceptions for quiet NaN arguments.  (I'm presuming that the
output of these functions goes through folding that converts
__builtin_isless to !UNGE_EXPR, etc.)

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..d8ff9c70ae6b9e72e09b8cbd9a0bd41b6830b83e 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -160,7 +160,6 @@ static tree fold_builtin_0 (location_t, tree);
 static tree fold_builtin_1 (location_t, tree, tree);
 static tree fold_builtin_2 (location_t, tree, tree, tree);
 static tree fold_builtin_3 (location_t, tree, tree, tree, tree);
-static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
 static tree fold_builtin_strstr (location_t, tree, tree, tree);
@@ -2202,19 +2201,8 @@ interclass_mathfn_icode (tree arg, tree fndecl)
   switch (DECL_FUNCTION_CODE (fndecl))
 {
 CASE_FLT_FN (BUILT_IN_ILOGB):
-  errno_set = true; builtin_optab = ilogb_optab; break;
-CASE_FLT_FN (BUILT_IN_ISINF):
-  builtin_optab = isinf_optab; break;
-case BUILT_IN_ISNORMAL:
-case BUILT_IN_ISFINITE:
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_FINITED32:
-case BUILT_IN_FINITED64:
-case BUILT_IN_FINITED128:
-case BUILT_IN_ISINFD32:
-case BUILT_IN_ISINFD64:
-case BUILT_IN_ISINFD128:
-  /* These builtins have no optabs (yet).  */
+  errno_set = true;
+  builtin_optab = ilogb_optab;
   break;
 default:
   gcc_unreachable ();
@@ -2233,8 +2221,7 @@ interclass_mathfn_icode (tree arg, tree fndecl)
 }
 
 /* Expand a call to one of the builtin math functions that operate on
-   floating point argument and output an integer result (ilogb, isinf,
-   isnan, etc).
+   floating point argument and output an integer result (ilogb, etc).
Return 0 if a normal call should be emitted rather than expanding the
function in-line.  EXP is the expression that is a call to the builtin
function; if convenient, the result should be placed in TARGET.  */
@@ -5997,11 +5984,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 CASE_FLT_FN (BUILT_IN_ILOGB):
   if (! flag_unsafe_math_optimizations)
 	break;
-  gcc_fallthrough ();
-CASE_FLT_FN (BUILT_IN_ISINF):
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_ISFINITE:
-case BUILT_IN_ISNORMAL:
+
   target = expand_builtin_interclass_mathfn (exp, target);
   if (target)
 	return target;
@@ -6281,8 +6264,25 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 	}
   break;
 
+CASE_FLT_FN (BUILT_IN_ISINF):
+case BUILT_IN_ISNAND32:
+case BUILT_IN_ISNAND64:
+case BUILT_IN_ISNAND128:
+case BUILT_IN_ISNAN:
+case BUILT_IN_ISINFD32:
+case BUILT_IN_ISINFD64:
+case BUILT_IN_ISINFD128:
+case BUILT_IN_ISNORMAL:
+case BUILT_IN_ISZERO:
+case BUILT_IN_ISSUBNORMAL:
+case BUILT_IN_FPCLASSIFY:
 case BUILT_IN_SETJMP:
-  /* This should have been lowered to the builtins below.  */
+CASE_FLT_FN (BUILT_IN_FINITE):
+case BUILT_IN_FINITED32:
+case BUILT_IN_FINITED64:
+case BUILT_IN_FINITED128:
+case BUILT_IN_ISFINITE:
+  /* T

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 19 Jan 2017, Tamar Christina wrote:

> > The old code set orig_arg before converting IBM long double to double.
> > Your code sets it after the conversion.  The old code set min_exp based on
> > a string set from REAL_MODE_FORMAT (orig_mode)->emin - 1; your code uses
> > the adjusted mode.  Both of those are incorrect for IBM long double.
> 
> Hmm, this is correct, I've made the change to be like it was before, but
> there's something I don't quite get about the old code, if it's building rmin
> the orig_mode which is larger than mode, but then creating the real using
> build_real (type, rmin) using the adjusted type, shouldn't it not be getting
> truncated?

The value is a power of 2, which is *larger* than the minimum normal value 
for double (as they have the same least subnormal value).

Looking further at the code, my only remaining comments are for the cases 
where you aren't using the integer path: in is_normal you use LT_EXPR to 
compare with +Inf, but need to use __builtin_isless, likewise in 
is_subnormal again you should be using __builtin_isless and 
__builtin_isgreater, in is_finite you should be using 
__builtin_islessequal.  All the existing expressions will raise spurious 
"invalid" exceptions for quiet NaN arguments.  (I'm presuming that the 
output of these functions goes through folding that converts 
__builtin_isless to !UNGE_EXPR, etc.)

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

> > The calls to is_zero and is_subnormal were incorrect indeed. I've
> > corrected them by not calling the fixup code and to instead make sure it
> > falls through into the old fp based code which did normal floating point
> > operations on the number. This is the same code as was before in
> > fpclassify so it should work.
>
> For is_zero it's fine to test based on the high part for IBM long double;
> an IBM long double is (zero, infinite, NaN, finite) if and only if the
> high part is.  The problem is the different threshold between normal and
> subnormal.

Ah ok, I've added it back in for is_zero then.

> > As for is_normal, the code is almost identical as the code that used to
> > be in fold_builtin_interclass_mathfn in BUILT_IN_ISNORMAL, with the
> > exception that I don't check <= max_value but instead < inifity, so I
> > can reuse the same constant.
>
> The old code set orig_arg before converting IBM long double to double.
> Your code sets it after the conversion.  The old code set min_exp based on
> a string set from REAL_MODE_FORMAT (orig_mode)->emin - 1; your code uses
> the adjusted mode.  Both of those are incorrect for IBM long double.

Hmm, this is correct, I've made the change to be like it was before, but
there's something I don't quite get about the old code, if it's building rmin
the orig_mode which is larger than mode, but then creating the real using
build_real (type, rmin) using the adjusted type, shouldn't it not be getting
truncated?

I've updated and attached the patch.

Tamar


From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, January 19, 2017 2:46:06 PM
To: Tamar Christina
Cc: Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; 
nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 19 Jan 2017, Tamar Christina wrote:

> > Also, I don't think the call to perform_ibm_extended_fixups in
> > is_subnormal is correct.  Subnormal for IBM long double is *not* the same
> > as subnormal double high part.  Likewise it's incorrect in is_normal as
> > well.
>
> The calls to is_zero and is_subnormal were incorrect indeed. I've
> corrected them by not calling the fixup code and to instead make sure it
> falls through into the old fp based code which did normal floating point
> operations on the number. This is the same code as was before in
> fpclassify so it should work.

For is_zero it's fine to test based on the high part for IBM long double;
an IBM long double is (zero, infinite, NaN, finite) if and only if the
high part is.  The problem is the different threshold between normal and
subnormal.

> As for is_normal, the code is almost identical as the code that used to
> be in fold_builtin_interclass_mathfn in BUILT_IN_ISNORMAL, with the
> exception that I don't check <= max_value but instead < inifity, so I
> can reuse the same constant.

The old code set orig_arg before converting IBM long double to double.
Your code sets it after the conversion.  The old code set min_exp based on
a string set from REAL_MODE_FORMAT (orig_mode)->emin - 1; your code uses
the adjusted mode.  Both of those are incorrect for IBM long double.

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..d8ff9c70ae6b9e72e09b8cbd9a0bd41b6830b83e 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -160,7 +160,6 @@ static tree fold_builtin_0 (location_t, tree);
 static tree fold_builtin_1 (location_t, tree, tree);
 static tree fold_builtin_2 (location_t, tree, tree, tree);
 static tree fold_builtin_3 (location_t, tree, tree, tree, tree);
-static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
 static tree fold_builtin_strstr (location_t, tree, tree, tree);
@@ -2202,19 +2201,8 @@ interclass_mathfn_icode (tree arg, tree fndecl)
   switch (DECL_FUNCTION_CODE (fndecl))
 {
 CASE_FLT_FN (BUILT_IN_ILOGB):
-  errno_set = true; builtin_optab = ilogb_optab; break;
-CASE_FLT_FN (BUILT_IN_ISINF):
-  builtin_optab = isinf_optab; break;
-case BUILT_IN_ISNORMAL:
-case BUILT_IN_ISFINITE:
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_FINITED32:
-case BUILT_IN_FINITED64:
-case BUILT_IN_FINITED128:
-case BUILT_IN_ISINFD32:
-case BUILT_IN_ISINFD64:
-case BUILT_IN_ISINFD128:
-  /* These builtins have no optabs (yet).  */
+  errno_set = true;
+  builtin_optab = ilogb_optab;
   break;
 default:
   gcc_unreachable ();
@@ -2233,8 +2221,7 @@ interclass_mathfn_icode (tree arg, tree fndecl)
 }
 
 /* Expand a call to one of the builtin math functions that operate on
-   floating point argument and output an integer result (ilogb, is

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 19 Jan 2017, Tamar Christina wrote:

> > Also, I don't think the call to perform_ibm_extended_fixups in
> > is_subnormal is correct.  Subnormal for IBM long double is *not* the same
> > as subnormal double high part.  Likewise it's incorrect in is_normal as
> > well.
> 
> The calls to is_zero and is_subnormal were incorrect indeed. I've 
> corrected them by not calling the fixup code and to instead make sure it 
> falls through into the old fp based code which did normal floating point 
> operations on the number. This is the same code as was before in 
> fpclassify so it should work.

For is_zero it's fine to test based on the high part for IBM long double; 
an IBM long double is (zero, infinite, NaN, finite) if and only if the 
high part is.  The problem is the different threshold between normal and 
subnormal.

> As for is_normal, the code is almost identical as the code that used to 
> be in fold_builtin_interclass_mathfn in BUILT_IN_ISNORMAL, with the 
> exception that I don't check <= max_value but instead < inifity, so I 
> can reuse the same constant.

The old code set orig_arg before converting IBM long double to double.  
Your code sets it after the conversion.  The old code set min_exp based on 
a string set from REAL_MODE_FORMAT (orig_mode)->emin - 1; your code uses 
the adjusted mode.  Both of those are incorrect for IBM long double.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

_half_format): Likewise.
(real_internal_format): Likewise.
* gcc/doc/extend.texi: Added documentation for built-ins.
* gcc/c/c-typeck.c (convert_arguments): Added BUILT_IN_ISZERO
and BUILT_IN_ISSUBNORMAL.

gcc/testsuite/
2017-01-19  Tamar Christina  <tamar.christ...@arm.com>

* gcc.target/aarch64/builtin-fpclassify.c: New codegen test.
* gcc.dg/fold-notunord.c: Removed.
* gcc.dg/torture/floatn-tg-4.h: Added tests for iszero and issubnormal.
* gcc.dg/torture/float128-tg-4.c: Likewise.
* gcc.dg/torture/float128x-tg-4: Likewise.
* gcc.dg/torture/float16-tg-4.c: Likewise.
* gcc.dg/torture/float32-tg-4.c: Likewise.
* gcc.dg/torture/float32x-tg-4.c: Likewise.
* gcc.dg/torture/float64-tg-4.c: Likewise.
* gcc.dg/torture/float64x-tg-4.c: Likewise.
* gcc.dg/pr28796-1.c: Added -O2.
* gcc.dg/builtins-43.c: Check lower instead of gimple.
* gcc/testsuite/gcc.dg/tg-tests.h: Added iszero and issubnormal.

Tamar.


From: Joseph Myers <jos...@codesourcery.com>
Sent: Wednesday, January 18, 2017 5:05:07 PM
To: Jeff Law
Cc: Tamar Christina; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Wed, 18 Jan 2017, Joseph Myers wrote:

> Generally, I don't see tests added that these new functions are correct
> for float, double and long double, which would detect such issues if run
> for a target with IBM long double.

Specifically, I think gcc.dg/tg-tests.h should have tests added for
__builtin_issubnormal and __builtin_iszero (i.e. have the existing test
inputs also tested with the new functions).

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..d8ff9c70ae6b9e72e09b8cbd9a0bd41b6830b83e 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -160,7 +160,6 @@ static tree fold_builtin_0 (location_t, tree);
 static tree fold_builtin_1 (location_t, tree, tree);
 static tree fold_builtin_2 (location_t, tree, tree, tree);
 static tree fold_builtin_3 (location_t, tree, tree, tree, tree);
-static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
 static tree fold_builtin_strstr (location_t, tree, tree, tree);
@@ -2202,19 +2201,8 @@ interclass_mathfn_icode (tree arg, tree fndecl)
   switch (DECL_FUNCTION_CODE (fndecl))
 {
 CASE_FLT_FN (BUILT_IN_ILOGB):
-  errno_set = true; builtin_optab = ilogb_optab; break;
-CASE_FLT_FN (BUILT_IN_ISINF):
-  builtin_optab = isinf_optab; break;
-case BUILT_IN_ISNORMAL:
-case BUILT_IN_ISFINITE:
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_FINITED32:
-case BUILT_IN_FINITED64:
-case BUILT_IN_FINITED128:
-case BUILT_IN_ISINFD32:
-case BUILT_IN_ISINFD64:
-case BUILT_IN_ISINFD128:
-  /* These builtins have no optabs (yet).  */
+  errno_set = true;
+  builtin_optab = ilogb_optab;
   break;
 default:
   gcc_unreachable ();
@@ -2233,8 +2221,7 @@ interclass_mathfn_icode (tree arg, tree fndecl)
 }
 
 /* Expand a call to one of the builtin math functions that operate on
-   floating point argument and output an integer result (ilogb, isinf,
-   isnan, etc).
+   floating point argument and output an integer result (ilogb, etc).
Return 0 if a normal call should be emitted rather than expanding the
function in-line.  EXP is the expression that is a call to the builtin
function; if convenient, the result should be placed in TARGET.  */
@@ -5997,11 +5984,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 CASE_FLT_FN (BUILT_IN_ILOGB):
   if (! flag_unsafe_math_optimizations)
 	break;
-  gcc_fallthrough ();
-CASE_FLT_FN (BUILT_IN_ISINF):
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_ISFINITE:
-case BUILT_IN_ISNORMAL:
+
   target = expand_builtin_interclass_mathfn (exp, target);
   if (target)
 	return target;
@@ -6281,8 +6264,25 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 	}
   break;
 
+CASE_FLT_FN (BUILT_IN_ISINF):
+case BUILT_IN_ISNAND32:
+case BUILT_IN_ISNAND64:
+case BUILT_IN_ISNAND128:
+case BUILT_IN_ISNAN:
+case BUILT_IN_ISINFD32:
+case BUILT_IN_ISINFD64:
+case BUILT_IN_ISINFD128:
+case BUILT_IN_ISNORMAL:
+case BUILT_IN_ISZERO:
+case BUILT_IN_ISSUBNORMAL:
+case BUILT_IN_FPCLASSIFY:
 case BUILT_IN_SETJMP:
-  /* This should have been lowered to the builtins below.  */
+CASE_FLT_FN (BUILT_IN_FINITE):
+case BUILT_IN_FINITED32:
+case BUILT_IN_FINITED64:
+case BUILT_IN_FINITED128:
+case BUILT_IN_ISFINITE:
+  /* These should have been lowered to the builtins in gimple-low.c.  */
   gcc_unreacha

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Wed, 18 Jan 2017, Joseph Myers wrote:

> Generally, I don't see tests added that these new functions are correct 
> for float, double and long double, which would detect such issues if run 
> for a target with IBM long double.

Specifically, I think gcc.dg/tg-tests.h should have tests added for 
__builtin_issubnormal and __builtin_iszero (i.e. have the existing test 
inputs also tested with the new functions).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

Also, I don't think the call to perform_ibm_extended_fixups in 
is_subnormal is correct.  Subnormal for IBM long double is *not* the same 
as subnormal double high part.  Likewise it's incorrect in is_normal as 
well.

Generally, I don't see tests added that these new functions are correct 
for float, double and long double, which would detect such issues if run 
for a target with IBM long double.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

Since the patch adds new built-in functions __builtin_issubnormal and 
__builtin_iszero, it also needs to update c-typeck.c:convert_arguments to 
make those functions remove excess precision.  This is mentioned in the 
PRs 77925 and 77926 for addition of those functions (which as I noted in 
 should be 
included in the ChangeLog entry for the patch).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Jeff Law]

On 12/19/2016 10:18 AM, Tamar Christina wrote:

Hi All,

I've respun the patch with the feedback from Jeff and Joseph.


I think an integer mode should always exist - even in the case of TFmode
on 32-bit systems (32-bit sparc / s390, for example, use TFmode long
double for GNU/Linux, and it's supported as _Float128 and __float128 on
32-bit x86).  It just be not be usable for arithmetic or declaring
variables of that type.

You're right, so I test the integer mode I receive with scalar_mode_supported_p.
And this seems to do the right thing.

Thanks for all the comments so far!

Kind Regards,
Tamar

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, December 15, 2016 7:03:27 PM
To: Tamar Christina
Cc: Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; 
nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 15 Dec 2016, Tamar Christina wrote:


Note that on some systems we even disable 64bit floating point support.
I suspect this check needs a little re-thinking as I don't think that
checking for a specific UNITS_PER_WORD is correct, nor is checking the
width of the type.  I'm not offhand sure what the test should be, just
that I think we need something better here.

I think what I really wanted to test here is if there was an integer
mode available which has the exact width as the floating point one. So I
have replaced this with just a call to int_mode_for_mode. Which is
probably more correct.

I think an integer mode should always exist - even in the case of TFmode
on 32-bit systems (32-bit sparc / s390, for example, use TFmode long
double for GNU/Linux, and it's supported as _Float128 and __float128 on
32-bit x86).  It just be not be usable for arithmetic or declaring
variables of that type.

I don't know whether TImode bitwise operations, such as generated by this
fpclassify work, will get properly lowered to operations on supported
narrower modes, but I hope so (clearly it's simpler if you can write
things straightforwardly and have them cover this case of TFmode on 32-bit
systems automatically through lowering elsewhere in the compiler, than if
covering that case would require additional code - the more cases you
cover, the more opportunity there is for glibc to use the built-in
functions even with -fsignaling-nans).

--
Joseph S. Myers
jos...@codesourcery.com


fpclassify-patch-gimple-3.patch






diff --git a/gcc/gimple-low.c b/gcc/gimple-low.c
index 
64752b67b86b3d01df5f5661e4666df98b7b91d1..ceaee295c6e81531dbe047c569dd18332179ccbf
 100644
--- a/gcc/gimple-low.c
+++ b/gcc/gimple-low.c
@@ -30,6 +30,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "calls.h"
 #include "gimple-iterator.h"
 #include "gimple-low.h"
+#include "stor-layout.h"
+#include "target.h"
Presumably these are for the endianness & mode checking?  It's a bit of 
a wart checking those in gimple-low, but I can live with it.  We might 
consider ways to avoid this layering violation in the future.







+
+/* Validate a single argument ARG against a tree code CODE representing
+   a type.  */
I think this comment for gimple_validate_arg is outdated and superseded 
by the one immediately following.  So I think this comment can simply be 
removed.


With that nit fixed, this is OK for the trunk.  Thanks for your 
patience.  I'm really happy with the improvements that were made since 
the initial submission of this change.


jeff

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Ping. Does this still have a chance or should I table it till Stage 1 opens 
again?

Tamar.


From: Tamar Christina
Sent: Tuesday, January 3, 2017 9:55:51 AM
To: Jeff Law; Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Hi Jeff,

I wasn't sure if you saw the updated patch attached to the previous email or if 
you just hadn't had the time to look at it yet.

Cheers,
Tamar


From: Jeff Law <l...@redhat.com>
Sent: Monday, December 19, 2016 8:27:33 PM
To: Tamar Christina; Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On 12/15/2016 03:14 AM, Tamar Christina wrote:
>
>> On a high level, presumably there's no real value in keeping the old
>> code to "fold" fpclassify.  By exposing those operations as integer
>> logicals for the fast path, if the FP value becomes a constant during
>> the optimization pipeline we'll see the reinterpreted values flowing
>> into the new integer logical tests and they'll simplify just like
>> anything else.  Right?
>
> Yes, if it becomes a constant it will be folded away, both in the integer and 
> the fp case.
Thanks for clarifying.


>
>> The old IBM format is still supported, though they are expected to be
>> moveing towards a standard ieee 128 bit format.  So my only concern is
>> that we preserve correct behavior for those cases -- I don't really care
>> about optimizing them.  So I think you need to keep them.
>
> Yes, I re-added them. It's mostly a copy paste from what they were in the
> other functions. But I have no way of testing it.
Understood.

>>  > +  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
>>> +  return (format->is_binary_ieee_compatible
>>> +   && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
>>> +   /* We explicitly disable quad float support on 32 bit systems.  */
>>> +   && !(UNITS_PER_WORD == 4 && type_width == 128)
>>> +   && targetm.scalar_mode_supported_p (mode));
>>> +}
>> Presumably this is why you needed the target.h inclusion.
>>
>> Note that on some systems we even disable 64bit floating point support.
>> I suspect this check needs a little re-thinking as I don't think that
>> checking for a specific UNITS_PER_WORD is correct, nor is checking the
>> width of the type.  I'm not offhand sure what the test should be, just
>> that I think we need something better here.
>
> I think what I really wanted to test here is if there was an integer mode 
> available
> which has the exact width as the floating point one. So I have replaced this 
> with
> just a call to int_mode_for_mode. Which is probably more correct.
I'll need to think about it, but would inherently think that
int_mode_for_mode is better than an explicit check of UNITS_PER_WORD and
typewidth.


>
>>> +
>>> +/* Determines if the given number is a NaN value.
>>> +   This function is the last in the chain and only has to
>>> +   check if it's preconditions are true.  */
>>> +static tree
>>> +is_nan (gimple_seq *seq, tree arg, location_t loc)
>> So in the old code we checked UNGT_EXPR, in the new code's slow path you
>> check UNORDERED.  Was that change intentional?
>
> The old FP code used UNORDERED and the new one was using ORDERED and negating 
> the result.
> I've replaced it with UNORDERED, but both are correct.
OK.  Just wanted to make sure.

jeff

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi Jeff,

I wasn't sure if you saw the updated patch attached to the previous email or if 
you just hadn't had the time to look at it yet.

Cheers,
Tamar


From: Jeff Law <l...@redhat.com>
Sent: Monday, December 19, 2016 8:27:33 PM
To: Tamar Christina; Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On 12/15/2016 03:14 AM, Tamar Christina wrote:
>
>> On a high level, presumably there's no real value in keeping the old
>> code to "fold" fpclassify.  By exposing those operations as integer
>> logicals for the fast path, if the FP value becomes a constant during
>> the optimization pipeline we'll see the reinterpreted values flowing
>> into the new integer logical tests and they'll simplify just like
>> anything else.  Right?
>
> Yes, if it becomes a constant it will be folded away, both in the integer and 
> the fp case.
Thanks for clarifying.


>
>> The old IBM format is still supported, though they are expected to be
>> moveing towards a standard ieee 128 bit format.  So my only concern is
>> that we preserve correct behavior for those cases -- I don't really care
>> about optimizing them.  So I think you need to keep them.
>
> Yes, I re-added them. It's mostly a copy paste from what they were in the
> other functions. But I have no way of testing it.
Understood.

>>  > +  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
>>> +  return (format->is_binary_ieee_compatible
>>> +   && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
>>> +   /* We explicitly disable quad float support on 32 bit systems.  */
>>> +   && !(UNITS_PER_WORD == 4 && type_width == 128)
>>> +   && targetm.scalar_mode_supported_p (mode));
>>> +}
>> Presumably this is why you needed the target.h inclusion.
>>
>> Note that on some systems we even disable 64bit floating point support.
>> I suspect this check needs a little re-thinking as I don't think that
>> checking for a specific UNITS_PER_WORD is correct, nor is checking the
>> width of the type.  I'm not offhand sure what the test should be, just
>> that I think we need something better here.
>
> I think what I really wanted to test here is if there was an integer mode 
> available
> which has the exact width as the floating point one. So I have replaced this 
> with
> just a call to int_mode_for_mode. Which is probably more correct.
I'll need to think about it, but would inherently think that
int_mode_for_mode is better than an explicit check of UNITS_PER_WORD and
typewidth.


>
>>> +
>>> +/* Determines if the given number is a NaN value.
>>> +   This function is the last in the chain and only has to
>>> +   check if it's preconditions are true.  */
>>> +static tree
>>> +is_nan (gimple_seq *seq, tree arg, location_t loc)
>> So in the old code we checked UNGT_EXPR, in the new code's slow path you
>> check UNORDERED.  Was that change intentional?
>
> The old FP code used UNORDERED and the new one was using ORDERED and negating 
> the result.
> I've replaced it with UNORDERED, but both are correct.
OK.  Just wanted to make sure.

jeff

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Jeff Law]

On 12/15/2016 03:14 AM, Tamar Christina wrote:



On a high level, presumably there's no real value in keeping the old
code to "fold" fpclassify.  By exposing those operations as integer
logicals for the fast path, if the FP value becomes a constant during
the optimization pipeline we'll see the reinterpreted values flowing
into the new integer logical tests and they'll simplify just like
anything else.  Right?


Yes, if it becomes a constant it will be folded away, both in the integer and 
the fp case.

Thanks for clarifying.





The old IBM format is still supported, though they are expected to be
moveing towards a standard ieee 128 bit format.  So my only concern is
that we preserve correct behavior for those cases -- I don't really care
about optimizing them.  So I think you need to keep them.


Yes, I re-added them. It's mostly a copy paste from what they were in the
other functions. But I have no way of testing it.

Understood.


 > +  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);

+  return (format->is_binary_ieee_compatible
+   && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
+   /* We explicitly disable quad float support on 32 bit systems.  */
+   && !(UNITS_PER_WORD == 4 && type_width == 128)
+   && targetm.scalar_mode_supported_p (mode));
+}

Presumably this is why you needed the target.h inclusion.

Note that on some systems we even disable 64bit floating point support.
I suspect this check needs a little re-thinking as I don't think that
checking for a specific UNITS_PER_WORD is correct, nor is checking the
width of the type.  I'm not offhand sure what the test should be, just
that I think we need something better here.


I think what I really wanted to test here is if there was an integer mode 
available
which has the exact width as the floating point one. So I have replaced this 
with
just a call to int_mode_for_mode. Which is probably more correct.
I'll need to think about it, but would inherently think that 
int_mode_for_mode is better than an explicit check of UNITS_PER_WORD and 
typewidth.






+
+/* Determines if the given number is a NaN value.
+   This function is the last in the chain and only has to
+   check if it's preconditions are true.  */
+static tree
+is_nan (gimple_seq *seq, tree arg, location_t loc)

So in the old code we checked UNGT_EXPR, in the new code's slow path you
check UNORDERED.  Was that change intentional?


The old FP code used UNORDERED and the new one was using ORDERED and negating 
the result.
I've replaced it with UNORDERED, but both are correct.

OK.  Just wanted to make sure.

jeff

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi All,

I've respun the patch with the feedback from Jeff and Joseph.

> I think an integer mode should always exist - even in the case of TFmode
> on 32-bit systems (32-bit sparc / s390, for example, use TFmode long
> double for GNU/Linux, and it's supported as _Float128 and __float128 on
> 32-bit x86).  It just be not be usable for arithmetic or declaring
> variables of that type.

You're right, so I test the integer mode I receive with scalar_mode_supported_p.
And this seems to do the right thing.

Thanks for all the comments so far!

Kind Regards,
Tamar

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, December 15, 2016 7:03:27 PM
To: Tamar Christina
Cc: Jeff Law; GCC Patches; Wilco Dijkstra; rguent...@suse.de; Michael Meissner; 
nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 15 Dec 2016, Tamar Christina wrote:

> > Note that on some systems we even disable 64bit floating point support.
> > I suspect this check needs a little re-thinking as I don't think that
> > checking for a specific UNITS_PER_WORD is correct, nor is checking the
> > width of the type.  I'm not offhand sure what the test should be, just
> > that I think we need something better here.
>
> I think what I really wanted to test here is if there was an integer
> mode available which has the exact width as the floating point one. So I
> have replaced this with just a call to int_mode_for_mode. Which is
> probably more correct.

I think an integer mode should always exist - even in the case of TFmode
on 32-bit systems (32-bit sparc / s390, for example, use TFmode long
double for GNU/Linux, and it's supported as _Float128 and __float128 on
32-bit x86).  It just be not be usable for arithmetic or declaring
variables of that type.

I don't know whether TImode bitwise operations, such as generated by this
fpclassify work, will get properly lowered to operations on supported
narrower modes, but I hope so (clearly it's simpler if you can write
things straightforwardly and have them cover this case of TFmode on 32-bit
systems automatically through lowering elsewhere in the compiler, than if
covering that case would require additional code - the more cases you
cover, the more opportunity there is for glibc to use the built-in
functions even with -fsignaling-nans).

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..d8ff9c70ae6b9e72e09b8cbd9a0bd41b6830b83e 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -160,7 +160,6 @@ static tree fold_builtin_0 (location_t, tree);
 static tree fold_builtin_1 (location_t, tree, tree);
 static tree fold_builtin_2 (location_t, tree, tree, tree);
 static tree fold_builtin_3 (location_t, tree, tree, tree, tree);
-static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
 static tree fold_builtin_strstr (location_t, tree, tree, tree);
@@ -2202,19 +2201,8 @@ interclass_mathfn_icode (tree arg, tree fndecl)
   switch (DECL_FUNCTION_CODE (fndecl))
 {
 CASE_FLT_FN (BUILT_IN_ILOGB):
-  errno_set = true; builtin_optab = ilogb_optab; break;
-CASE_FLT_FN (BUILT_IN_ISINF):
-  builtin_optab = isinf_optab; break;
-case BUILT_IN_ISNORMAL:
-case BUILT_IN_ISFINITE:
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_FINITED32:
-case BUILT_IN_FINITED64:
-case BUILT_IN_FINITED128:
-case BUILT_IN_ISINFD32:
-case BUILT_IN_ISINFD64:
-case BUILT_IN_ISINFD128:
-  /* These builtins have no optabs (yet).  */
+  errno_set = true;
+  builtin_optab = ilogb_optab;
   break;
 default:
   gcc_unreachable ();
@@ -2233,8 +2221,7 @@ interclass_mathfn_icode (tree arg, tree fndecl)
 }
 
 /* Expand a call to one of the builtin math functions that operate on
-   floating point argument and output an integer result (ilogb, isinf,
-   isnan, etc).
+   floating point argument and output an integer result (ilogb, etc).
Return 0 if a normal call should be emitted rather than expanding the
function in-line.  EXP is the expression that is a call to the builtin
function; if convenient, the result should be placed in TARGET.  */
@@ -5997,11 +5984,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 CASE_FLT_FN (BUILT_IN_ILOGB):
   if (! flag_unsafe_math_optimizations)
 	break;
-  gcc_fallthrough ();
-CASE_FLT_FN (BUILT_IN_ISINF):
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_ISFINITE:
-case BUILT_IN_ISNORMAL:
+
   target = expand_builtin_interclass_mathfn (exp, target);
   if (target)
 	return target;
@@ -6281,8 +6264,25 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 	}
   break;
 
+CASE_FLT_FN (BUILT_IN_ISINF):
+case BUILT_IN_ISNAND32:
+ 

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 15 Dec 2016, Tamar Christina wrote:

> > Note that on some systems we even disable 64bit floating point support.
> > I suspect this check needs a little re-thinking as I don't think that
> > checking for a specific UNITS_PER_WORD is correct, nor is checking the
> > width of the type.  I'm not offhand sure what the test should be, just
> > that I think we need something better here.
> 
> I think what I really wanted to test here is if there was an integer 
> mode available which has the exact width as the floating point one. So I 
> have replaced this with just a call to int_mode_for_mode. Which is 
> probably more correct.

I think an integer mode should always exist - even in the case of TFmode 
on 32-bit systems (32-bit sparc / s390, for example, use TFmode long 
double for GNU/Linux, and it's supported as _Float128 and __float128 on 
32-bit x86).  It just be not be usable for arithmetic or declaring 
variables of that type.

I don't know whether TImode bitwise operations, such as generated by this 
fpclassify work, will get properly lowered to operations on supported 
narrower modes, but I hope so (clearly it's simpler if you can write 
things straightforwardly and have them cover this case of TFmode on 32-bit 
systems automatically through lowering elsewhere in the compiler, than if 
covering that case would require additional code - the more cases you 
cover, the more opportunity there is for glibc to use the built-in 
functions even with -fsignaling-nans).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

> On a high level, presumably there's no real value in keeping the old
> code to "fold" fpclassify.  By exposing those operations as integer
> logicals for the fast path, if the FP value becomes a constant during
> the optimization pipeline we'll see the reinterpreted values flowing
> into the new integer logical tests and they'll simplify just like
> anything else.  Right?

Yes, if it becomes a constant it will be folded away, both in the integer and 
the fp case.

> The old IBM format is still supported, though they are expected to be
> moveing towards a standard ieee 128 bit format.  So my only concern is
> that we preserve correct behavior for those cases -- I don't really care
> about optimizing them.  So I think you need to keep them.

Yes, I re-added them. It's mostly a copy paste from what they were in the
other functions. But I have no way of testing it.

> For documenting builtins, using existing builtins as a template.

Yeah, I based them off the fpclassify documentation.

> > +{
> > +  tree type = TREE_TYPE (arg);
> > +
> > +  machine_mode mode = TYPE_MODE (type);
> > +
> > +  const real_format *format = REAL_MODE_FORMAT (mode);
>  > +  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
> > +  return (format->is_binary_ieee_compatible
> > +   && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
> > +   /* We explicitly disable quad float support on 32 bit systems.  */
> > +   && !(UNITS_PER_WORD == 4 && type_width == 128)
> > +   && targetm.scalar_mode_supported_p (mode));
> > +}
> Presumably this is why you needed the target.h inclusion.
>
> Note that on some systems we even disable 64bit floating point support.
> I suspect this check needs a little re-thinking as I don't think that
> checking for a specific UNITS_PER_WORD is correct, nor is checking the
> width of the type.  I'm not offhand sure what the test should be, just
> that I think we need something better here.

I think what I really wanted to test here is if there was an integer mode 
available
which has the exact width as the floating point one. So I have replaced this 
with
just a call to int_mode_for_mode. Which is probably more correct.

> > +
> > +/* Determines if the given number is a NaN value.
> > +   This function is the last in the chain and only has to
> > +   check if it's preconditions are true.  */
> > +static tree
> > +is_nan (gimple_seq *seq, tree arg, location_t loc)
> So in the old code we checked UNGT_EXPR, in the new code's slow path you
> check UNORDERED.  Was that change intentional?

The old FP code used UNORDERED and the new one was using ORDERED and negating 
the result.
I've replaced it with UNORDERED, but both are correct.

Thanks for the review,
I'll get the new patch out ASAP.

Tamar

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Jeff Law]

On 11/25/2016 05:18 AM, Tamar Christina wrote:

Hi Joseph,

I have updated the patch with the changes,
w.r.t to the formatting, there are tabs there that seem to be rendered
at 4 spaces wide. In my editor setup at 8 spaces they are correct.

Various random comments, mostly formatting.  I do think we're going to 
need another iteration.  The good news is I don't expect we'll be asking 
for major changes in direction, just trying to tie up various loose 
ends, so it shouldn't take nearly as long.


On a high level, presumably there's no real value in keeping the old 
code to "fold" fpclassify.  By exposing those operations as integer 
logicals for the fast path, if the FP value becomes a constant during 
the optimization pipeline we'll see the reinterpreted values flowing 
into the new integer logical tests and they'll simplify just like 
anything else.  Right?


The old IBM format is still supported, though they are expected to be 
moveing towards a standard ieee 128 bit format.  So my only concern is 
that we preserve correct behavior for those cases -- I don't really care 
about optimizing them.  So I think you need to keep them.


For documenting builtins, using existing builtins as a template.




@@ -822,6 +882,736 @@ lower_builtin_setjmp (gimple_stmt_iterator *gsi)
   gsi_remove (gsi, false);
 }

+static tree
+emit_tree_and_return_var (gimple_seq *seq, tree arg)

Needs a function comment.


+{
+  if (TREE_CODE (arg) == SSA_NAME || VAR_P (arg))
+return arg;
+
+  tree tmp = create_tmp_reg (TREE_TYPE(arg));

Nit.  Need space between TREE_TYPE and the open-parn for its arglist


+  gassign *stm = gimple_build_assign(tmp, arg);
Similarly between gimple_build_assign and its arglist.  This formatting 
nit is repeated often.  Please check the patch as a whole and correct. 
Probably the best way to go is a search for [a-z] immediately preceding 
an open paren.




+/* This function builds an if statement that ends up using explicit branches
+   instead of becoming a csel.  This function assumes you will fall through to
+   the next statements after this condition for the false branch.  */
A function comment is supposed to document each argument (and the return 
value if any).  It's probably better to avoid referring to an ARM 
instruction (csel) and instead describe the intent more genericly.




+static void
+emit_tree_cond (gimple_seq *seq, tree result_variable, tree exit_label,
+   tree cond, tree true_branch)
+{
+/* Create labels for fall through.  */
+  tree true_label = create_artificial_label (UNKNOWN_LOCATION);

Comment is indented too deep.  It should line up with the code.



+
+static tree
+get_num_as_int (gimple_seq *seq, tree arg, location_t loc)
Needs function comment.  I'm not going to call out each one.  Please 
verify that every new function has a comment and that they document 
their arguments and return values.


Here's an example from builtins.c you might want to refer back to:


/* For a memory reference expression EXP compute values M and N such that M
   divides ( - N) and such that N < M.  If these numbers can be determined,
   store M in alignp and N in *BITPOSP and return true.  Otherwise return false
   and store BITS_PER_UNIT to *alignp and any bit-offset to *bitposp.  */

bool
get_object_alignment_1 (tree exp, unsigned int *alignp,
unsigned HOST_WIDE_INT *bitposp)
{
  return get_object_alignment_2 (exp, alignp, bitposp, false);
}





+  /* Check if the number that is being classified is close enough to IEEE 754
+ format to be able to go in the early exit code.  */
+static bool
+use_ieee_int_mode (tree arg)
Comment should describe the argument.  Comment is indented 2 spaces too 
deep on both lines.  This occurs in several please.  I won't call each 
one out, but please walk through the patch as a whole and fix them.




+{
+  tree type = TREE_TYPE (arg);
+
+  machine_mode mode = TYPE_MODE (type);
+
+  const real_format *format = REAL_MODE_FORMAT (mode);
+  const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
+  return (format->is_binary_ieee_compatible
+ && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
+ /* We explicitly disable quad float support on 32 bit systems.  */
+ && !(UNITS_PER_WORD == 4 && type_width == 128)
+ && targetm.scalar_mode_supported_p (mode));
+}

Presumably this is why you needed the target.h inclusion.

Note that on some systems we even disable 64bit floating point support. 
I suspect this check needs a little re-thinking as I don't think that 
checking for a specific UNITS_PER_WORD is correct, nor is checking the 
width of the type.  I'm not offhand sure what the test should be, just 
that I think we need something better here.





+
+static tree
+is_normal (gimple_seq *seq, tree arg, location_t loc)
+{
+  tree type = TREE_TYPE (arg);
+
+  machine_mode mode = TYPE_MODE (type);
+  const real_format *format = REAL_MODE_FORMAT (mode);
+  const tree bool_type = boolean_type_node;
+

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Jeff Law]

On 12/12/2016 02:19 AM, Tamar Christina wrote:

Ping


From: Tamar Christina
Sent: Friday, December 2, 2016 4:20:42 PM
To: Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Ping?

Is there anything else I need to do for the patch or is it OK for trunk?
Just a note, it is in my queue of things to look at.   GIven it was 
posted well before stage1 close I think it deserves the opportunity to 
move forward (even if we need another iteration) even though we're well 
into stage3.


Jeff

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Ping


From: Tamar Christina
Sent: Friday, December 2, 2016 4:20:42 PM
To: Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Ping?

Is there anything else I need to do for the patch or is it OK for trunk?

Thanks,
Tamar


From: Tamar Christina
Sent: Friday, November 25, 2016 12:18:52 PM
To: Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Hi Joseph,

I have updated the patch with the changes,
w.r.t to the formatting, there are tabs there that seem to be rendered
at 4 spaces wide. In my editor setup at 8 spaces they are correct.

Kind Regards,
Tamar

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, November 24, 2016 6:28:18 PM
To: Tamar Christina
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 24 Nov 2016, Tamar Christina wrote:

> @@ -11499,6 +11503,53 @@ to classify.  GCC treats the last argument as 
> type-generic, which
>  means it does not do default promotion from float to double.
>  @end deftypefn
>
> +@deftypefn {Built-in Function} int __builtin_isnan (...)
> +This built-in implements the C99 isnan functionality which checks if
> +the given argument represents a NaN.  The return value of the
> +function will either be a 0 (false) or a 1 (true).
> +On most systems, when an IEEE 754 floating point is used this
> +built-in does not produce a signal when a signaling NaN is used.

"an IEEE 754 floating point" should probably be "an IEEE 754
floating-point type" or similar.

> +GCC treats the argument as type-generic, which means it does
> +not do default promotion from float to double.

I think type names such as float and double should use @code{} in the
manual.

> +the given argument represents an Infinite number.  The return

Infinite should not have a capital letter there.

> +@deftypefn {Built-in Function} int __builtin_iszero (...)
> +This built-in implements the C99 iszero functionality which checks if

This isn't C99, it's TS 18661-1:2014.

> +the given argument represents the number 0.  The return

0 or -0.

> +@deftypefn {Built-in Function} int __builtin_issubnormal (...)
> +This built-in implements the C99 issubnormal functionality which checks if

Again, TS 18661-1.

> +the given argument represents a sub-normal number.  The return

Do not hyphenate subnormal.

> + switch (DECL_FUNCTION_CODE (decl))
> + {
> + case BUILT_IN_SETJMP:
> + lower_builtin_setjmp (gsi);
> + data->cannot_fallthru = false;
> + return;

The indentation in this whole block of code (not all quoted) is wrong.

> +real_inf();

Missing space before '('.

> +  emit_tree_cond (, dest, done_label,
> +   is_normal(, arg, loc), fp_normal);
> +  emit_tree_cond (, dest, done_label,
> +   is_zero(, arg, loc), fp_zero);
> +  emit_tree_cond (, dest, done_label,
> +   is_nan(, arg, loc), fp_nan);
> +  emit_tree_cond (, dest, done_label,
> +   is_infinity(, arg, loc), fp_infinite);

Likewise.

> +   fndecl(, arg, loc), t_true);

Likewise.

--
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Ping?

Is there anything else I need to do for the patch or is it OK for trunk?

Thanks,
Tamar


From: Tamar Christina
Sent: Friday, November 25, 2016 12:18:52 PM
To: Joseph Myers
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

Hi Joseph,

I have updated the patch with the changes,
w.r.t to the formatting, there are tabs there that seem to be rendered
at 4 spaces wide. In my editor setup at 8 spaces they are correct.

Kind Regards,
Tamar

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, November 24, 2016 6:28:18 PM
To: Tamar Christina
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 24 Nov 2016, Tamar Christina wrote:

> @@ -11499,6 +11503,53 @@ to classify.  GCC treats the last argument as 
> type-generic, which
>  means it does not do default promotion from float to double.
>  @end deftypefn
>
> +@deftypefn {Built-in Function} int __builtin_isnan (...)
> +This built-in implements the C99 isnan functionality which checks if
> +the given argument represents a NaN.  The return value of the
> +function will either be a 0 (false) or a 1 (true).
> +On most systems, when an IEEE 754 floating point is used this
> +built-in does not produce a signal when a signaling NaN is used.

"an IEEE 754 floating point" should probably be "an IEEE 754
floating-point type" or similar.

> +GCC treats the argument as type-generic, which means it does
> +not do default promotion from float to double.

I think type names such as float and double should use @code{} in the
manual.

> +the given argument represents an Infinite number.  The return

Infinite should not have a capital letter there.

> +@deftypefn {Built-in Function} int __builtin_iszero (...)
> +This built-in implements the C99 iszero functionality which checks if

This isn't C99, it's TS 18661-1:2014.

> +the given argument represents the number 0.  The return

0 or -0.

> +@deftypefn {Built-in Function} int __builtin_issubnormal (...)
> +This built-in implements the C99 issubnormal functionality which checks if

Again, TS 18661-1.

> +the given argument represents a sub-normal number.  The return

Do not hyphenate subnormal.

> + switch (DECL_FUNCTION_CODE (decl))
> + {
> + case BUILT_IN_SETJMP:
> + lower_builtin_setjmp (gsi);
> + data->cannot_fallthru = false;
> + return;

The indentation in this whole block of code (not all quoted) is wrong.

> +real_inf();

Missing space before '('.

> +  emit_tree_cond (, dest, done_label,
> +   is_normal(, arg, loc), fp_normal);
> +  emit_tree_cond (, dest, done_label,
> +   is_zero(, arg, loc), fp_zero);
> +  emit_tree_cond (, dest, done_label,
> +   is_nan(, arg, loc), fp_nan);
> +  emit_tree_cond (, dest, done_label,
> +   is_infinity(, arg, loc), fp_infinite);

Likewise.

> +   fndecl(, arg, loc), t_true);

Likewise.

--
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi Joseph,

I have updated the patch with the changes,
w.r.t to the formatting, there are tabs there that seem to be rendered
at 4 spaces wide. In my editor setup at 8 spaces they are correct.

Kind Regards,
Tamar

From: Joseph Myers <jos...@codesourcery.com>
Sent: Thursday, November 24, 2016 6:28:18 PM
To: Tamar Christina
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Thu, 24 Nov 2016, Tamar Christina wrote:

> @@ -11499,6 +11503,53 @@ to classify.  GCC treats the last argument as 
> type-generic, which
>  means it does not do default promotion from float to double.
>  @end deftypefn
>
> +@deftypefn {Built-in Function} int __builtin_isnan (...)
> +This built-in implements the C99 isnan functionality which checks if
> +the given argument represents a NaN.  The return value of the
> +function will either be a 0 (false) or a 1 (true).
> +On most systems, when an IEEE 754 floating point is used this
> +built-in does not produce a signal when a signaling NaN is used.

"an IEEE 754 floating point" should probably be "an IEEE 754
floating-point type" or similar.

> +GCC treats the argument as type-generic, which means it does
> +not do default promotion from float to double.

I think type names such as float and double should use @code{} in the
manual.

> +the given argument represents an Infinite number.  The return

Infinite should not have a capital letter there.

> +@deftypefn {Built-in Function} int __builtin_iszero (...)
> +This built-in implements the C99 iszero functionality which checks if

This isn't C99, it's TS 18661-1:2014.

> +the given argument represents the number 0.  The return

0 or -0.

> +@deftypefn {Built-in Function} int __builtin_issubnormal (...)
> +This built-in implements the C99 issubnormal functionality which checks if

Again, TS 18661-1.

> +the given argument represents a sub-normal number.  The return

Do not hyphenate subnormal.

> + switch (DECL_FUNCTION_CODE (decl))
> + {
> + case BUILT_IN_SETJMP:
> + lower_builtin_setjmp (gsi);
> + data->cannot_fallthru = false;
> + return;

The indentation in this whole block of code (not all quoted) is wrong.

> +real_inf();

Missing space before '('.

> +  emit_tree_cond (, dest, done_label,
> +   is_normal(, arg, loc), fp_normal);
> +  emit_tree_cond (, dest, done_label,
> +   is_zero(, arg, loc), fp_zero);
> +  emit_tree_cond (, dest, done_label,
> +   is_nan(, arg, loc), fp_nan);
> +  emit_tree_cond (, dest, done_label,
> +   is_infinity(, arg, loc), fp_infinite);

Likewise.

> +   fndecl(, arg, loc), t_true);

Likewise.

--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 3ac2d44148440b124559ba7cd3de483b7a74b72d..2340f60edb31ebf964367699aaf33ac8401dff41 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -160,7 +160,6 @@ static tree fold_builtin_0 (location_t, tree);
 static tree fold_builtin_1 (location_t, tree, tree);
 static tree fold_builtin_2 (location_t, tree, tree, tree);
 static tree fold_builtin_3 (location_t, tree, tree, tree, tree);
-static tree fold_builtin_varargs (location_t, tree, tree*, int);
 
 static tree fold_builtin_strpbrk (location_t, tree, tree, tree);
 static tree fold_builtin_strstr (location_t, tree, tree, tree);
@@ -2202,19 +2201,8 @@ interclass_mathfn_icode (tree arg, tree fndecl)
   switch (DECL_FUNCTION_CODE (fndecl))
 {
 CASE_FLT_FN (BUILT_IN_ILOGB):
-  errno_set = true; builtin_optab = ilogb_optab; break;
-CASE_FLT_FN (BUILT_IN_ISINF):
-  builtin_optab = isinf_optab; break;
-case BUILT_IN_ISNORMAL:
-case BUILT_IN_ISFINITE:
-CASE_FLT_FN (BUILT_IN_FINITE):
-case BUILT_IN_FINITED32:
-case BUILT_IN_FINITED64:
-case BUILT_IN_FINITED128:
-case BUILT_IN_ISINFD32:
-case BUILT_IN_ISINFD64:
-case BUILT_IN_ISINFD128:
-  /* These builtins have no optabs (yet).  */
+  errno_set = true;
+  builtin_optab = ilogb_optab;
   break;
 default:
   gcc_unreachable ();
@@ -2233,8 +2221,7 @@ interclass_mathfn_icode (tree arg, tree fndecl)
 }
 
 /* Expand a call to one of the builtin math functions that operate on
-   floating point argument and output an integer result (ilogb, isinf,
-   isnan, etc).
+   floating point argument and output an integer result (ilogb, etc).
Return 0 if a normal call should be emitted rather than expanding the
function in-line.  EXP is the expression that is a call to the builtin
function; if convenient, the result should be placed in TARGET.  */
@@ -5997,11 +5984,7 @@ expand_builtin (tree e

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Thu, 24 Nov 2016, Tamar Christina wrote:

> @@ -11499,6 +11503,53 @@ to classify.  GCC treats the last argument as 
> type-generic, which
>  means it does not do default promotion from float to double.
>  @end deftypefn
>  
> +@deftypefn {Built-in Function} int __builtin_isnan (...)
> +This built-in implements the C99 isnan functionality which checks if
> +the given argument represents a NaN.  The return value of the
> +function will either be a 0 (false) or a 1 (true).
> +On most systems, when an IEEE 754 floating point is used this
> +built-in does not produce a signal when a signaling NaN is used.

"an IEEE 754 floating point" should probably be "an IEEE 754 
floating-point type" or similar.

> +GCC treats the argument as type-generic, which means it does
> +not do default promotion from float to double.

I think type names such as float and double should use @code{} in the 
manual.

> +the given argument represents an Infinite number.  The return

Infinite should not have a capital letter there.

> +@deftypefn {Built-in Function} int __builtin_iszero (...)
> +This built-in implements the C99 iszero functionality which checks if

This isn't C99, it's TS 18661-1:2014.

> +the given argument represents the number 0.  The return

0 or -0.

> +@deftypefn {Built-in Function} int __builtin_issubnormal (...)
> +This built-in implements the C99 issubnormal functionality which checks if

Again, TS 18661-1.

> +the given argument represents a sub-normal number.  The return

Do not hyphenate subnormal.

> + switch (DECL_FUNCTION_CODE (decl))
> + {
> + case BUILT_IN_SETJMP:
> + lower_builtin_setjmp (gsi);
> + data->cannot_fallthru = false;
> + return;

The indentation in this whole block of code (not all quoted) is wrong.

> +real_inf();

Missing space before '('.

> +  emit_tree_cond (, dest, done_label,
> +   is_normal(, arg, loc), fp_normal);
> +  emit_tree_cond (, dest, done_label,
> +   is_zero(, arg, loc), fp_zero);
> +  emit_tree_cond (, dest, done_label,
> +   is_nan(, arg, loc), fp_nan);
> +  emit_tree_cond (, dest, done_label,
> +   is_infinity(, arg, loc), fp_infinite);

Likewise.

> +   fndecl(, arg, loc), t_true);

Likewise.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Tamar Christina]

Hi All,

This is a respin of the v3 of the patch which adds an optimized route to the 
fpclassify builtin
for floating point numbers which are similar to IEEE-754 in format.

Compared to the last one this adds the tests requested for the FloatN types,
adds documentation, ISFINITE and IBM extended support back in.

Regression tests ran on aarch64-none-linux and arm-none-linux-gnueabi
and no regression. x86_64 bootstrapped successfully as well.

I would appreciate if someone with access to a machine with the IBM extended
types can test this as well.

Ok for trunk?

Thanks,
Tamar

gcc/
2016-11-17  Tamar Christina  <tamar.christ...@arm.com>

PR middle-end/77925
PR middle-end/77926
PR middle-end/66462

* gcc/builtins.c (fold_builtin_fpclassify): Removed.
(fold_builtin_interclass_mathfn): Removed.
(expand_builtin): Added builtins to lowering list.
(fold_builtin_n): Removed fold_builtin_varargs.
(fold_builtin_varargs): Removed.
* gcc/builtins.def (BUILT_IN_ISZERO, BUILT_IN_ISSUBNORMAL): Added.
* gcc/real.h (get_min_float): Added.
(real_format): Added is_ieee_compatible field.
* gcc/real.c (get_min_float): Added.
(ieee_single_format): Set is_ieee_compatible flag.
* gcc/gimple-low.c (lower_stm): Define BUILT_IN_FPCLASSIFY,
CASE_FLT_FN (BUILT_IN_ISINF), BUILT_IN_ISINFD32, BUILT_IN_ISINFD64,
BUILT_IN_ISINFD128, BUILT_IN_ISNAND32, BUILT_IN_ISNAND64,
BUILT_IN_ISNAND128, BUILT_IN_ISNAN, BUILT_IN_ISNORMAL, BUILT_IN_ISZERO,
BUILT_IN_ISSUBNORMAL, CASE_FLT_FN (BUILT_IN_FINITE), BUILT_IN_FINITED32
BUILT_IN_FINITED64, BUILT_IN_FINITED128, BUILT_IN_ISFINITE.
(lower_builtin_fpclassify, is_nan, is_normal, is_infinity): Added.
(is_zero, is_subnormal, is_finite, use_ieee_int_mode): Likewise.
(lower_builtin_isnan, lower_builtin_isinfinite): Likewise.
(lower_builtin_isnormal, lower_builtin_iszero): Likewise.
(lower_builtin_issubnormal, lower_builtin_isfinite): Likewise.
(emit_tree_cond, get_num_as_int, emit_tree_and_return_var): Added.
(mips_single_format): Likewise.
(motorola_single_format): Likewise.
(spu_single_format): Likewise.
(ieee_double_format): Likewise.
(mips_double_format): Likewise.
(motorola_double_format): Likewise.
(ieee_extended_motorola_format): Likewise.
(ieee_extended_intel_128_format): Likewise.
(ieee_extended_intel_96_round_53_format): Likewise.
(ibm_extended_format): Likewise.
(mips_extended_format): Likewise.
(ieee_quad_format): Likewise.
(mips_quad_format): Likewise.
(vax_f_format): Likewise.
(vax_d_format): Likewise.
(vax_g_format): Likewise.
(decimal_single_format): Likewise.
(decimal_quad_format): Likewise.
(iee_half_format): Likewise.
(mips_single_format): Likewise.
(arm_half_format): Likewise.
(real_internal_format): Likewise.
* gcc/doc/extend.texi: Added documentation for built-ins.

gcc/testsuite/
2016-11-17  Tamar Christina  <tamar.christ...@arm.com>

* gcc.target/aarch64/builtin-fpclassify.c: New codegen test.
* gcc.dg/fold-notunord.c: Removed.
* gcc.dg/torture/floatn-tg-4.h: Added tests for iszero and issubnormal.
* gcc.dg/torture/float128-tg-4.c: Likewise.
* gcc.dg/torture/float128x-tg-4: Likewise.
* gcc.dg/torture/float16-tg-4.c: Likewise.
* gcc.dg/torture/float32-tg-4.c: Likewise.
* gcc.dg/torture/float32x-tg-4.c: Likewise.
* gcc.dg/torture/float64-tg-4.c: Likewise.
* gcc.dg/torture/float64x-tg-4.c: Likewise.
* gcc.dg/pr28796-1.c: Added -O2.
* gcc.dg/builtins-43.c: Check lower instead of gimple.

From: Joseph Myers <jos...@codesourcery.com>
Sent: Friday, November 11, 2016 10:05:43 PM
To: Tamar Christina
Cc: GCC Patches; Wilco Dijkstra; rguent...@suse.de; l...@redhat.com; Michael 
Meissner; nd
Subject: Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers 
in GIMPLE.

On Fri, 11 Nov 2016, Tamar Christina wrote:

> is_infinite and is_zero have been created. This patch also introduces two new
> intrinsics __builtin_iszero and __builtin_issubnormal.

And so the ChangeLog entry needs to include:

PR middle-end/77925
PR middle-end/77926

(in addition to PR references for whatever PRs for signaling NaN issues
are partly addressed by the patch - those can't be closed however until
fully fixed for all formats with signaling NaNs).

> NOTE: the old code for ISNORMAL, ISSUBNORMAL, ISNAN and ISINFINITE had a
>   special case for ibm_extended_format which dropped the second part
>   of the number (which was being represented as two numbers internally).
>   fpclassify did not have such a case. As such I have dropped it 

Re: [PATCH][GCC][PATCHv3] Improve fpclassify w.r.t IEEE like numbers in GIMPLE.

[Joseph Myers]

On Fri, 11 Nov 2016, Tamar Christina wrote:

> is_infinite and is_zero have been created. This patch also introduces two new
> intrinsics __builtin_iszero and __builtin_issubnormal.

And so the ChangeLog entry needs to include:

PR middle-end/77925
PR middle-end/77926

(in addition to PR references for whatever PRs for signaling NaN issues 
are partly addressed by the patch - those can't be closed however until 
fully fixed for all formats with signaling NaNs).

> NOTE: the old code for ISNORMAL, ISSUBNORMAL, ISNAN and ISINFINITE had a
>   special case for ibm_extended_format which dropped the second part
>   of the number (which was being represented as two numbers internally).
>   fpclassify did not have such a case. As such I have dropped it as I am
>   under the impression the format is deprecated? So the optimization isn't
>   as important? If this is wrong it would be easy to add that back in.

It's not simply an optimization when it involves comparison against the 
largest normal value (to determine whether something is finite / infinite) 
- in that case it's needed to avoid bad results on the true maximum value 
(mantissa 53 1s, 0, 53 1s) which can occur at runtime but GCC can't 
represent internally because it internally treats this format as having a 
fixed width of 106 bits.

> Should ISFINITE be change as well? Also should it be SUBNORMAL or DENORMAL?

It's subnormal.  (Denormal was the old name in IEEE 754-1985.)

> And what should I do about Documentation? I'm not sure how to document a new
> BUILTIN.

Document them in the "Other Builtins" node in extend.texi, like the 
existing classification built-in functions.

> diff --git a/gcc/builtins.def b/gcc/builtins.def
> index 
> 219feebd3aebefbd079bf37cc801453cd1965e00..e3d12eccfed528fd6df0570b65f8aef42494d675
>  100644
> --- a/gcc/builtins.def
> +++ b/gcc/builtins.def
> @@ -831,6 +831,8 @@ DEF_EXT_LIB_BUILTIN(BUILT_IN_ISINFL, "isinfl", 
> BT_FN_INT_LONGDOUBLE, ATTR_CO
>  DEF_EXT_LIB_BUILTIN(BUILT_IN_ISINFD32, "isinfd32", BT_FN_INT_DFLOAT32, 
> ATTR_CONST_NOTHROW_LEAF_LIST)
>  DEF_EXT_LIB_BUILTIN(BUILT_IN_ISINFD64, "isinfd64", BT_FN_INT_DFLOAT64, 
> ATTR_CONST_NOTHROW_LEAF_LIST)
>  DEF_EXT_LIB_BUILTIN(BUILT_IN_ISINFD128, "isinfd128", 
> BT_FN_INT_DFLOAT128, ATTR_CONST_NOTHROW_LEAF_LIST)
> +DEF_C99_C90RES_BUILTIN (BUILT_IN_ISZERO, "iszero", BT_FN_INT_VAR, 
> ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
> +DEF_C99_C90RES_BUILTIN (BUILT_IN_ISSUBNORMAL, "issubnormal", BT_FN_INT_VAR, 
> ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)

No, these should be DEF_GCC_BUILTIN, so that only the __builtin_* names 
exist.

> +static tree
> +is_zero (gimple_seq *seq, tree arg, location_t loc)
> +{
> +  tree type = TREE_TYPE (arg);
> +
> +  /* If not using optimized route then exit early.  */
> +  if (!use_ieee_int_mode (arg))
> +  {
> +tree arg_p
> +  = emit_tree_and_return_var (seq, fold_build1_loc (loc, ABS_EXPR, type,
> + arg));
> +tree res = fold_build2_loc (loc, EQ_EXPR, boolean_type_node, arg_p,
> + build_real (type, dconst0));

There is no need to take the absolute value before comparing with constant 
0.

I think tests for the new built-in functions should be added for the 
_Float* types (so add gcc.dg/torture/float-tg-4.h to test them and 
associated float*-tg-4.c files using that header to instantiate the tests 
for each type).

-- 
Joseph S. Myers
jos...@codesourcery.com