public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Michael Meissner <meissner@linux.ibm.com>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Michael Meissner <meissner@linux.ibm.com>,
	gcc-patches@gcc.gnu.org, David Edelsohn <dje.gcc@gmail.com>,
	Bill Schmidt <wschmidt@linux.ibm.com>,
	Peter Bergner <bergner@linux.ibm.com>
Subject: Re: [PATCH] PowerPC: Map IEEE 128-bit long double built-in functions
Date: Fri, 11 Dec 2020 17:07:28 -0500	[thread overview]
Message-ID: <20201211220728.GA25584@ibm-toto.the-meissners.org> (raw)
In-Reply-To: <20201210212001.GW2672@gate.crashing.org>

On Thu, Dec 10, 2020 at 03:20:01PM -0600, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Nov 19, 2020 at 06:58:14PM -0500, Michael Meissner wrote:
> > 	* config/rs6000/rs6000.c (rs6000_mangle_decl_assembler_name): Add
> > 	support for mapping built-in function names for long double
> > 	built-in functions if long double is IEEE 128-bit.
> 
> Please write what it does, not "add support".  Say what names it maps
> to, importantly.  You don't need to list all, but what you wrote is
> 100% contentless.

Ok.

> > diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> > index a5188553593..35e9c844e17 100644
> > --- a/gcc/config/rs6000/rs6000.c
> > +++ b/gcc/config/rs6000/rs6000.c
> > @@ -27065,57 +27065,128 @@ rs6000_globalize_decl_name (FILE * stream, tree decl)
> >     library before you can switch the real*16 type at compile time.
> >  
> >     We use the TARGET_MANGLE_DECL_ASSEMBLER_NAME hook to change this name.  We
> > -   only do this if the default is that long double is IBM extended double, and
> > -   the user asked for IEEE 128-bit.  */
> > +   only do this transformation if the __float128 type is enabled.  This
> > +   prevents us from doing the transformation on older 32-bit ports that might
> > +   have enabled using IEEE 128-bit floating point as the default long double
> > +   type.  */
> 
> I still don't understand why you want to support some hypothetical and
> untested configuration.

I'm not sure what you mean by hypothetical and untested configuration.  If you
build a default configuration on a big endian system (i.e. do not use
--with-cpu specifying at least power7), the __float128 type is not enabled by
default, because you need access to the VSX vector registers.

> > +  /* { dg-final { scan-assembler {\mbl __ynieee128} } }  */
> 
> This kind of thing does not portably work (the function names can have
> various prefixes added).

Why would it have prefixes?  Can you suggest a better way of doing the test?
There are lots of different long names that need to get mapped if long doubles
are IEEE 128-bit.  This test just tries to test every function to make sure it
got mapped appropriately.

We can't do a link test without requiring that GLIBC is 2.32 or newer, since
the older GLIBCs don't have all of the symbols.  And the link test would only
work on little endian ELFV2 systems, since big endian GLIBC 2.32 does not
export any of the float128 stuff.

> I cannot understand this code, and it does seem far from obviously
> correct.  But, okay for trunk if you handle all fallout (and I mean all,
> not just "all you consider important").

Note, the code is only invokved for systems where IEEE 128-bit floating point
is supported.  So it will never get called for a lot of the random embedded
systems or the big endian systems.

There is another way I could do the code that is simpler, but over time it will
need maintainence as new built-in functions are added.

Right now, in going over the system built-ins, it tries to do the mapping based
on the names:

    1)	If the function has a long double argument or returns long double, and
	it ends in 'l', the name is mapped to "__" followed by <name minus 'l'>
	and then "ieee128".  I.e. "sinl" becomes "__sinieee128".

    2)	However, some names do not follow that formula and I have a switch
	statement for the outliers.

    3)	If the function name ends in "printf", the name is transformed to "__"
	followed by <name> followed by "ieee128".  I.e. "sprintf" becomes
	"__sprintfieee128".

    4)	If the function names ends in "scanf", the name is transformed to
	"__isoc99_" followed by <name> followed by "ieee128".  I.e. "vscanf"
	becomes "__isoc99_vscanfieee128".

I could change it to just having a switch statement of all known built-in
functions.  I would need to add some code that if checking was enabled, it
would look for built-in functions that use long double that are not mapped, and
issue a warning to update the table of maps.  This might be cleaner to look at,
but every so often, we would need to add a new symbol.

Would you prefer me to do that second implementation?

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

  reply	other threads:[~2020-12-11 22:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19 23:58 Michael Meissner
2020-12-04  4:31 ` Ping " Michael Meissner
2020-12-10 15:27 ` Ping x2: " Michael Meissner
2020-12-10 21:20 ` Segher Boessenkool
2020-12-11 22:07   ` Michael Meissner [this message]
2020-12-11 23:03     ` Segher Boessenkool

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=20201211220728.GA25584@ibm-toto.the-meissners.org \
    --to=meissner@linux.ibm.com \
    --cc=bergner@linux.ibm.com \
    --cc=dje.gcc@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=segher@kernel.crashing.org \
    --cc=wschmidt@linux.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).