Patchwork RFA (mmix): FIx PR46739

login
register
mail settings
Submitter Joern Rennecke
Date Dec. 6, 2010, 5:04 p.m.
Message ID <20101206120402.jqbfggfnk0gk4kgs-nzlynne@webmail.spamcop.net>
Download mbox | patch
Permalink /patch/74396/
State New
Headers show

Comments

Joern Rennecke - Dec. 6, 2010, 5:04 p.m.
Cross-built on x86_64-pc-linux-gnu.
2010-11-29  Joern Rennecke  <amylaar@spamcop.net>

	PR target/46739
	* config/mmix/mmix.c (mmix_intval): Split shift count.
Richard Henderson - Dec. 6, 2010, 5:16 p.m.
> 	PR target/46739
> 	* config/mmix/mmix.c (mmix_intval): Split shift count.

Ok.


r~
Hans-Peter Nilsson - Dec. 6, 2010, 5:18 p.m.
On Mon, 6 Dec 2010, Joern Rennecke wrote:

> Cross-built on x86_64-pc-linux-gnu.
(not quotable from within pine, but no specific need this time)

Ok, thanks!  Feel free to CC me on CRIS and MMIX issues; I'm
frequently behind on my mailing list reading.

brgds, H-P
Joseph S. Myers - Dec. 6, 2010, 5:21 p.m.
I don't think the code is correct either before or after this patch; 
aren't the halves of a CONST_DOUBLE in units of HOST_WIDE_INT not long?  
The whole block

  /* We make a little song and dance because converting to long long in
     gcc-2.7.2 is broken.  I still want people to be able to use it for
     cross-compilation to MMIX.  */
  if (GET_CODE (x) == CONST_DOUBLE && GET_MODE (x) == VOIDmode)
    {
      if (sizeof (HOST_WIDE_INT) < sizeof (HOST_WIDEST_INT))
        {
          retval = (unsigned) CONST_DOUBLE_LOW (x) / 2;
          retval *= 2;
          retval |= CONST_DOUBLE_LOW (x) & 1;

          retval |=
            (unsigned HOST_WIDEST_INT) CONST_DOUBLE_HIGH (x)
              << (HOST_BITS_PER_LONG);
        }
      else
        retval = CONST_DOUBLE_HIGH (x);

      return retval;
    }

should be substantially simplified given that 2.95 not 2.7.2 is the 
minimum version for building a cross compiler.  (Maybe double-int.[ch] 
should have functions to convert a double_int value to either signed or 
unsigned HOST_WIDEST_INT.)
Richard Henderson - Dec. 6, 2010, 5:28 p.m.
On 12/06/2010 09:21 AM, Joseph S. Myers wrote:
> I don't think the code is correct either before or after this patch; 
> aren't the halves of a CONST_DOUBLE in units of HOST_WIDE_INT not long?  

Yes, they are.

> (Maybe double-int.[ch] 
> should have functions to convert a double_int value to either signed or 
> unsigned HOST_WIDEST_INT.)

Sure.  double_int_to_{s,u}widest{,_p}?


r~
Joern Rennecke - Dec. 6, 2010, 6:23 p.m.
Quoting "Joseph S. Myers" <joseph@codesourcery.com>:

> I don't think the code is correct either before or after this patch;
> aren't the halves of a CONST_DOUBLE in units of HOST_WIDE_INT not long?

That is true; but this is really a separate issue.  Note, for this to
break, there have to be two distinct type sizes larger than that of long.
Also worrying is the cast of the low part to unsigned, that will even break
if we have int / long / long long (or int128) as 32 / 64 / 128 bit.

> The whole block
...
> should be substantially simplified given that 2.95 not 2.7.2 is the
> minimum version for building a cross compiler.

It looks as if this was a specific decision to support 2.7.2 by the port
maintainer, albeit that was a while ago.  I'll leave it to the port
maintainers to decide if this is now obsolete.
Hans-Peter Nilsson - Dec. 6, 2010, 8:15 p.m.
On Mon, 6 Dec 2010, Joern Rennecke wrote:
> Quoting "Joseph S. Myers" <joseph@codesourcery.com>:
> > I don't think the code is correct either before or after this patch;
> > aren't the halves of a CONST_DOUBLE in units of HOST_WIDE_INT not long?
>
> That is true; but this is really a separate issue.  Note, for this to
> break, there have to be two distinct type sizes larger than that of long.
> Also worrying is the cast of the low part to unsigned, that will even break
> if we have int / long / long long (or int128) as 32 / 64 / 128 bit.
>
> > The whole block
> ...
> > should be substantially simplified given that 2.95 not 2.7.2 is the
> > minimum version for building a cross compiler.
>
> It looks as if this was a specific decision to support 2.7.2 by the port
> maintainer, albeit that was a while ago.  I'll leave it to the port
> maintainers to decide if this is now obsolete.

...well, quite...  :-)

(No, I didn't consider the context of the patch and as you say,
it's a separate issue.  Thanks everyone for caring.)

brgds, H-P
Joseph S. Myers - Dec. 6, 2010, 8:28 p.m.
On Mon, 6 Dec 2010, Joern Rennecke wrote:

> > should be substantially simplified given that 2.95 not 2.7.2 is the
> > minimum version for building a cross compiler.
> 
> It looks as if this was a specific decision to support 2.7.2 by the port
> maintainer, albeit that was a while ago.  I'll leave it to the port
> maintainers to decide if this is now obsolete.

It's obviously obsolete, since ports can't unilaterally support older host 
compilers than the rest of GCC.
Joern Rennecke - Dec. 6, 2010, 11:19 p.m.
Quoting "Joseph S. Myers" <joseph@codesourcery.com>:

> On Mon, 6 Dec 2010, Joern Rennecke wrote:
>
>> > should be substantially simplified given that 2.95 not 2.7.2 is the
>> > minimum version for building a cross compiler.
>>
>> It looks as if this was a specific decision to support 2.7.2 by the port
>> maintainer, albeit that was a while ago.  I'll leave it to the port
>> maintainers to decide if this is now obsolete.
>
> It's obviously obsolete, since ports can't unilaterally support older host
> compilers than the rest of GCC.

It's not quite that obvious.  When we say we require a minimum version of
GCC, we may say this because of same part that is not built for all targets,
or for which a miscompilation might be a non-issue for some target as the
code is never reached.
Or some issues might be host-specific.
Some issues might be sporadic, triggered by particular code patterns,
and very time consuming to specify what host/target combinations are
affected.  The hack in mmix_intval looks like it works around an issue
in this category.

Patch

Index: config/mmix/mmix.c
===================================================================
--- config/mmix/mmix.c	(revision 167494)
+++ config/mmix/mmix.c	(working copy)
@@ -2747,7 +2747,7 @@  mmix_intval (rtx x)
 
 	  retval |=
 	    (unsigned HOST_WIDEST_INT) CONST_DOUBLE_HIGH (x)
-	      << (HOST_BITS_PER_LONG);
+	      << (HOST_BITS_PER_LONG)/2 << (HOST_BITS_PER_LONG)/2;
 	}
       else
 	retval = CONST_DOUBLE_HIGH (x);