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

[Tamar Christina <Tamar dot Christina at arm dot com>]

> -----Original Message-----
> From: Richard Biener [mailto:rguenther@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
> <joseph@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
	fcvt	d0, s0
	fcmp	d0, 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.