RFA (mmix): FIx PR46739

Submitted by Joern Rennecke on Dec. 6, 2010, 5:04 p.m.

Details

Message ID 20101206120402.jqbfggfnk0gk4kgs-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

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.

Comments

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 hide | download patch | download mbox

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