This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]