Patchwork [fortran] Use BUILT_IN_IROUND

login
register
mail settings
Submitter Janne Blomqvist
Date March 16, 2012, 8:17 a.m.
Message ID <CAO9iq9Gq4Sntc=nppWwGkrVyNNssqXYz9=AJ0JTFPi18esYM1w@mail.gmail.com>
Download mbox | patch
Permalink /patch/147152/
State New
Headers show

Comments

Janne Blomqvist - March 16, 2012, 8:17 a.m.
On Thu, Mar 15, 2012 at 22:28, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Thu, Mar 15, 2012 at 22:14, Tobias Burnus <burnus@net-b.de> wrote:
>> Janne Blomqvist wrote:
>>>
>>> since some time GCC has BUILT_IN_IROUND{F,,L}, similar to lround() and
>>> llround() but the result is returned as an integer.
>>>
>>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>
>>
>> OK. Thanks for the patch! Nit: Could you check mathbuiltins.def - at least
>> in the diff, "iround" seems to be misaligned (one " " missing).
>
> Ah, a tab had sneaked in, fixed. Committed the fixed patch as r185442.
> Thanks for the quick review!
>
> --
> Janne Blomqvist

I realized there was a bug in the patch, and testing revealed my
suspicion as true. Namely as there is no library fallback for iround,
and the middle-end machinery takes care of converting it to lround
only for float, double, and long double, it doesn't work for
__float128. In the testcase in my original patch mail, changing x to
real(16) generated a call to iroundq, which doesn't exist. Committed
the patch below as obvious.

   else if (resprec <= LONG_TYPE_SIZE)
     fn = builtin_decl_for_precision (BUILT_IN_LROUND, argprec);
Richard Guenther - March 16, 2012, 11:21 a.m.
On Fri, Mar 16, 2012 at 9:17 AM, Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
> On Thu, Mar 15, 2012 at 22:28, Janne Blomqvist
> <blomqvist.janne@gmail.com> wrote:
>> On Thu, Mar 15, 2012 at 22:14, Tobias Burnus <burnus@net-b.de> wrote:
>>> Janne Blomqvist wrote:
>>>>
>>>> since some time GCC has BUILT_IN_IROUND{F,,L}, similar to lround() and
>>>> llround() but the result is returned as an integer.
>>>>
>>>> Regtested on x86_64-unknown-linux-gnu, Ok for trunk?
>>>
>>>
>>> OK. Thanks for the patch! Nit: Could you check mathbuiltins.def - at least
>>> in the diff, "iround" seems to be misaligned (one " " missing).
>>
>> Ah, a tab had sneaked in, fixed. Committed the fixed patch as r185442.
>> Thanks for the quick review!
>>
>> --
>> Janne Blomqvist
>
> I realized there was a bug in the patch, and testing revealed my
> suspicion as true. Namely as there is no library fallback for iround,
> and the middle-end machinery takes care of converting it to lround
> only for float, double, and long double, it doesn't work for
> __float128. In the testcase in my original patch mail, changing x to
> real(16) generated a call to iroundq, which doesn't exist. Committed
> the patch below as obvious.

This seems to have broken SPEC 2000 FPU quite a lot as we now get

/gcc/spec/sb-vangelis-head-64/x86_64/install-201203152354/lib/gcc/x86_64-unknown-linux-gnu/4.8.0/../../../../lib64/libgfortran.so:
undefined reference to `iroundq'
collect2: error: ld returned 1 exit status
specmake: *** [apsi] Error 1

though maybe it was Jakubs patch as libgfortran has the reference?

Jakub?

Richard.

> Index: ChangeLog
> ===================================================================
> --- ChangeLog   (revision 185452)
> +++ ChangeLog   (working copy)
> @@ -1,3 +1,8 @@
> +2012-03-16  Janne Blomqvist  <jb@gcc.gnu.org>
> +
> +       * trans-intrinsic.c (build_round_expr): Don't use BUILT_IN_IROUND
> +       for __float128.
> +
>  2012-03-15  Janne Blomqvist  <jb@gcc.gnu.org>
>
>        * f95-lang.c (gfc_init_builtin_functions): Initialize
> Index: trans-intrinsic.c
> ===================================================================
> --- trans-intrinsic.c   (revision 185452)
> +++ trans-intrinsic.c   (working copy)
> @@ -383,10 +383,11 @@ build_round_expr (tree arg, tree restype
>   resprec = TYPE_PRECISION (restype);
>
>   /* Depending on the type of the result, choose the int intrinsic
> -     (iround, available only as a builtin), long int intrinsic
> (lround
> -     family) or long long intrinsic (llround).  We might also need to
> -     convert the result afterwards.  */
> -  if (resprec <= INT_TYPE_SIZE)
> +     (iround, available only as a builtin, therefore cannot use it
> for
> +     __float128), long int intrinsic (lround family) or long long
> +     intrinsic (llround).  We might also need to convert the result
> +     afterwards.  */
> +  if (resprec <= INT_TYPE_SIZE && argprec <= LONG_DOUBLE_TYPE_SIZE)
>     fn = builtin_decl_for_precision (BUILT_IN_IROUND, argprec);
>   else if (resprec <= LONG_TYPE_SIZE)
>     fn = builtin_decl_for_precision (BUILT_IN_LROUND, argprec);
>
>
>
> --
> Janne Blomqvist

Patch

Index: ChangeLog
===================================================================
--- ChangeLog   (revision 185452)
+++ ChangeLog   (working copy)
@@ -1,3 +1,8 @@ 
+2012-03-16  Janne Blomqvist  <jb@gcc.gnu.org>
+
+       * trans-intrinsic.c (build_round_expr): Don't use BUILT_IN_IROUND
+       for __float128.
+
 2012-03-15  Janne Blomqvist  <jb@gcc.gnu.org>

        * f95-lang.c (gfc_init_builtin_functions): Initialize
Index: trans-intrinsic.c
===================================================================
--- trans-intrinsic.c   (revision 185452)
+++ trans-intrinsic.c   (working copy)
@@ -383,10 +383,11 @@  build_round_expr (tree arg, tree restype
   resprec = TYPE_PRECISION (restype);

   /* Depending on the type of the result, choose the int intrinsic
-     (iround, available only as a builtin), long int intrinsic
(lround
-     family) or long long intrinsic (llround).  We might also need to
-     convert the result afterwards.  */
-  if (resprec <= INT_TYPE_SIZE)
+     (iround, available only as a builtin, therefore cannot use it
for
+     __float128), long int intrinsic (lround family) or long long
+     intrinsic (llround).  We might also need to convert the result
+     afterwards.  */
+  if (resprec <= INT_TYPE_SIZE && argprec <= LONG_DOUBLE_TYPE_SIZE)
     fn = builtin_decl_for_precision (BUILT_IN_IROUND, argprec);