public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Ian Lance Taylor <iant@google.com>
To: Wolfgang Gellerich <gellerich@de.ibm.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] middle end, s390: optimization for builtin isnan
Date: Tue, 07 Aug 2007 16:29:00 -0000	[thread overview]
Message-ID: <m3sl6vnwcv.fsf@localhost.localdomain> (raw)
In-Reply-To: <OF9BDC2098.614F0753-ONC1257330.002FB977-42257330.004CD302@de.ibm.com>

Wolfgang Gellerich <gellerich@de.ibm.com> writes:

> Ian Lance Taylor <iant@google.com> wrote on 07.08.2007 03:26:28:
> 
> > Wolfgang Gellerich<gellerich@de.ibm.com> writes:
> >
> > > This patch introduces a new target hook allowing to optimize the
> > > implementation of builtin function isnan on platforms like s390 where
> > > arithmetic operations set the condition code when the result was NaN.
> It
> > > bootstraps on Intel and s390 and does not introduce new test case
> failures.
> >
> > It seems to me that we should handle this by treating isnan as a unary
> > operator which may be implemented in the MD file.  E.g., implement
> > isnansf1 and isnandf1, with optabs.  I don't see why a target hook is
> > the right thing here.
> 
> Hi Ian, many for your comments! However, what would we gain from
> implementing isnan as operator, and what do you consider as major
> disadvantages of using the new target hook? The reason why I chose the
> implementation I sent is that the current implementation was hard-wired at
> a rather early stage. I thought that there might be a reason for this, it
> may enable further optimizations in some cases or on some platforms. Also,
> the target hook approach has the advantage that is allows s390 (and other
> platforms) to add their optimizations while not causing any changes for
> platforms that are happy with the current implementation.

Sorry, you're right, my suggestion doesn't make sense.  Still, I'm not
that fond of target-specific target hooks.  Also, it's not obvious to
me that your suggestion is always an improvement.  It's clearly an
improvement for your test case, but what happens when there is no
floating point operation to set the condition code?  As in
    foo (double x) { return isnan (x); }
Don't you wind up materializing a floating point zero in order to do
the comparison, thus adding an unnecessary instruction?

Looking at your e-mail more closely, I expect that what you want can
be handle with a define_split which runs at combine time.  You want
the split to recognize the three instructions
    adbr f2, f3
    cdbr f2, f2
    jo label
and combine them into simply
    adbr f2, f3
    jo label
This combination of three instructions into two is what combine-time
define_splits do.

Ian

  reply	other threads:[~2007-08-07 16:29 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-08-03 12:54 Wolfgang Gellerich
2007-08-07  1:27 ` Ian Lance Taylor
2007-08-07 14:03   ` Wolfgang Gellerich
2007-08-07 16:29     ` Ian Lance Taylor [this message]
2007-08-09 13:46       ` Wolfgang Gellerich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m3sl6vnwcv.fsf@localhost.localdomain \
    --to=iant@google.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=gellerich@de.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).