GCC on SPARC64: umul bug

[Alexander Kabaev <ak03 at gte dot com>]

Building XFree86 4.3.x on FreeBSD/sparc64 uncovers the bug in how GCC
emits umul instruction in some cases. The following simple test case
shows the problem:

/* begin test case */
unsigned int rbytes;
unsigned int bytes_overflow;

void crash(unsigned long *bytes)
{
        *bytes = (bytes_overflow * 4294967295) + rbytes;
}

The incorrect assembler output is produced when this test case is
compiled on FreeBSD using default options, or on Solaris8 as follows:

% uname -a
SunOS sunnydale 5.8 Generic_108528-16 sun4u sparc SUNW,Sun-Fire-880

% gcc --version
gcc (GCC) 3.4 20030423 (experimental)
Copyright (C) 2002 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

% gcc -mcmodel=medlow -m64 -mcpu=ultrasparc -o umul -O2 umul.c
/usr/ccs/bin/as: "/var/tmp//ccBsmewk.s", line 15: error: constant value must be between -4096 and 4095

The assembly output looks like

        .file   "umul.c"
        .section        ".text"
        .align 4
        .align 32
        .global crash
        .type   crash, #function
        .proc   020
crash:
        !#PROLOGUE# 0
        !#PROLOGUE# 1
        sethi   %hi(bytes_overflow), %g4
        sethi   %hi(rbytes), %g5
        ld      [%g4+%lo(bytes_overflow)], %g1
        lduw    [%g5+%lo(rbytes)], %g4
        umul    %g1, 4294967295, %g1
        add     %g1, %g4, %g1
        retl
        stx     %g1, [%o0]
        .size   crash, .-crash


Tomas Moestl <tmm at FreeBSD dot ORG> did provide I patch to fix the bug.
Could someone please take a look and verify if his patch is correct?  I
bootstrapped GCC mainline on both FreeBSD and Solaris 8 with and without
this patch and saw no regressions. I'll post the Changelog entry and the
patch to the appropriate mailing list if someone confirm that the patch
is correct.

The original message from Tomas with his analysis and patch are below.
-- 
Alexander Kabaev

This is a arguably a gcc bug. All (13-bit) immediate operands are
sign-extended, even those to instructions which operate on unsigned
values, so umul can handle a range of very small and a range of very
large operands. gcc correctly recognizes that it can use an immediate
here; however, it chooses to output it as an unsigned number and does
not sign-extended it from 32 to 64 bit.

All sign extensions for instructions are made to the full 64 bit
however (even if umul only happens to use 32 of those), so when the
assembler checks whether a value is representable as an immediate, it
will check that the 64-bit sign extension of the immediate creates
the desired value (in sparc64 mode), i.e. it doesn't ignore the upper
32 bits even if a particular instruction does not use them.

One solution is to generate negative literals for immediates if we
mean them to be sign-extended (which gcc does already for some other
instructions). The attached patch implements this, I'm not sure it
uses the best possible way to do this though, and it also needs a bit
more testing.

	- Thomas

-- 
Thomas Moestl <tmoestl at gmx dot net>	http://www.tu-bs.de/~y0015675/
              <tmm at FreeBSD dot org>	http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0  9C0F 1FE6 4F1D 419C 776C

Index: config/sparc/sparc.c
===================================================================
RCS file: /ncvs/src/contrib/gcc/config/sparc/sparc.c,v
retrieving revision 1.1.1.9
diff -u -r1.1.1.9 sparc.c
--- config/sparc/sparc.c	10 Oct 2002 04:40:04 -0000	1.1.1.9
+++ config/sparc/sparc.c	16 Jan 2003 18:09:06 -0000
@@ -6462,6 +6462,22 @@
       output_address (XEXP (x, 0));
       return;
 
+    case 's':
+      {
+	/* Print a sign-extended 32-bit value.  */
+	HOST_WIDE_INT xi;
+	int i;
+	if (GET_CODE(x) == CONST_INT)
+	  xi = INTVAL (x);
+	else if (GET_CODE(x) == CONST_DOUBLE)
+	  xi = CONST_DOUBLE_LOW (x);
+	else
+	  output_operand_lossage ("invalid %%s operand");
+	i = trunc_int_for_mode (xi, SImode);
+	fprintf (file, "%d", i);
+	return;
+      }
+
     case 0:
       /* Do nothing special.  */
       break;
Index: config/sparc/sparc.md
===================================================================
RCS file: /ncvs/src/contrib/gcc/config/sparc/sparc.md,v
retrieving revision 1.1.1.8
diff -u -r1.1.1.8 sparc.md
--- config/sparc/sparc.md	10 Oct 2002 04:40:08 -0000	1.1.1.8
+++ config/sparc/sparc.md	16 Jan 2003 17:09:36 -0000
@@ -6120,7 +6120,7 @@
   "TARGET_HARD_MUL32"
   "*
 {
-  return TARGET_SPARCLET ? \"umuld\\t%1, %2, %L0\" : \"umul\\t%1, %2, %L0\\n\\trd\\t%%y, %H0\";
+  return TARGET_SPARCLET ? \"umuld\\t%1, %s2, %L0\" : \"umul\\t%1, %s2, %L0\\n\\trd\\t%%y, %H0\";
 }"
   [(set (attr "type")
 	(if_then_else (eq_attr "isa" "sparclet")
@@ -6134,7 +6134,7 @@
 	(mult:DI (zero_extend:DI (match_operand:SI 1 "register_operand" "r"))
 		 (match_operand:SI 2 "uns_small_int" "")))]
   "TARGET_DEPRECATED_V8_INSNS && TARGET_ARCH64"
-  "umul\\t%1, %2, %0"
+  "umul\\t%1, %s2, %0"
   [(set_attr "type" "imul")])
 
 ;; XXX
@@ -6145,8 +6145,8 @@
    (clobber (match_scratch:SI 3 "=X,h"))]
   "TARGET_V8PLUS"
   "@
-   umul\\t%1, %2, %L0\\n\\tsrlx\\t%L0, 32, %H0
-   umul\\t%1, %2, %3\\n\\tsrlx\\t%3, 32, %H0\\n\\tmov\\t%3, %L0"
+   umul\\t%1, %s2, %L0\\n\\tsrlx\\t%L0, 32, %H0
+   umul\\t%1, %s2, %3\\n\\tsrlx\\t%3, 32, %H0\\n\\tmov\\t%3, %L0"
   [(set_attr "type" "multi")
    (set_attr "length" "2,3")])