[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: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.

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
>         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.
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)