Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Steve Ellcey]

> Incidentally, the CONST_INT/CONST_DOUBLE case looks wrong.  We
> should be adjusting the constant according to POINTERS_EXTEND_UNSIGNED
> from from_mode to to_mode.  Really that means
> 
>   case CONST_INT: 
>   case CONST_DOUBLE:
>     if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode))
>       code = TRUNCATE;
>     else if (POINTERS_EXTEND_UNSIGNED)
>       code = ZERO_EXTEND;
>     else
>       code = SIGN_EXTEND;
>     return simplify_unary_operation (code, to_mode, x, from_mode);

Richard,

Following up to my own response, I think this change you are suggesting
will interact badly with the PLUS optimization on some platforms.  The
problem is that we have:

|    case PLUS:
|    case MULT:
|       /* For addition the second operand is a small constant, we can safely
|          permute the conversion and addition operation.  We can always safely
|          permute them if we are making the address narrower.  In addition,
|          always permute the operations if this is a constant.  */
|       if ((GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
|               || (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT
|                   && POINTERS_EXTEND_UNSIGNED < 0
|                   && (INTVAL (XEXP (x, 1)) + 20000 < 40000
|                       || CONSTANT_P (XEXP (x, 0))))))
|         return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
|                                convert_memory_address (to_mode, XEXP (x, 0)),
|                                convert_memory_address (to_mode, XEXP (x, 1)));
|       break;

Now, if we are truncating I don't think there is any problem, but if we
are extending a PLUS operator then we are calling convert_memory_address
with both sides of the PLUS and I think that is wrong because we are
really adding together an address and an offset, not two addresses and
if we zero-extended both the address and the offset and if the offset is
actually a negative CONST_INT won't that turn the offset into a large
positive number and won't that mean that when we add them together we
will get the wrong result compared with adding the two values together
first and then extending the result of the PLUS.

The full solution (I think) would be to add your CONST_INT/CONST_DOUBLE
change and fix this code to not call convert_memory_address on "XEXP (x,
1)" but do a call to simplify_unary_operation with SIGN_EXTEND
(regardless of the value of POINTERS_EXTEND_UNSIGNED, but I would like
your opinion on it.  I could just turn off the PLUS optimization on IA64
HP-UX (based on the value of POINTERS_EXTEND_UNSIGNED) and leave
everything else (CONST_INT/CONST_DOUBLE) alone and be happy.

Steve Ellcey
sje@cup.hp.com

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Richard Henderson]

On Wed, Jul 17, 2002 at 09:57:39AM -0700, Steve Ellcey wrote:
> Now, if we are truncating I don't think there is any problem, but if we
> are extending a PLUS operator then we are calling convert_memory_address
> with both sides of the PLUS and I think that is wrong because we are
> really adding together an address and an offset, not two addresses and
> if we zero-extended both the address and the offset and if the offset is
> actually a negative CONST_INT won't that turn the offset into a large
> positive number and won't that mean that when we add them together we
> will get the wrong result compared with adding the two values together
> first and then extending the result of the PLUS.

As I also said, the permutation is invalid if the constant is
changed by the conversion.  Thus one ought to have

  if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
      || (POINTERS_EXTEND_UNSIGNED >= 0
	  && GET_CODE (x) == PLUS
	  && GET_CODE (XEXP (x, 1)) == CONST_INT
	  && XEXP (x, 1) == convert_memory_address (to_mode, XEXP (x, 1))))
    return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
                           convert_memory_address (to_mode, XEXP (x, 0)),
			   XEXP (x, 1));


r~

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Richard Henderson]

On Mon, Jul 22, 2002 at 12:32:12PM -0700, Steve Ellcey wrote:
> Also, I was originally going to skip the permutation of the PLUS and the
> conversion when POINTERS_EXTEND_UNSIGNED < 0, but I found that I could
> not compile gcc.c-torture/compile/20010107-1.c in 32 bit mode without
> doing this optimization.  The compiler would die with:
> 
> 20010107-1.c:6: error: unrecognizable insn:
> (insn 36 29 37 0 00000000 (set (reg:DI 15 r15)
>         (plus:DI (reg:DI 1 r1)
>             (const:DI (reg:DI 340)))) -1 (nil)
>     (nil))
> 
> Since nothing failed when I did do this optimization in
> convert_memory_address I enabled it for IA64 HP-UX.

Hum.. I would like to understand who wrapped a register in a CONST.
It also seems amazing that you didn't run into problems with the
ptr_extension...

But I guess it's your bug to fix when you find something about it
that does break.  ;-)

Patch ok.


r~

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Steve Ellcey]

This is a patch to address a bug I found in convert_memory_address and
to fix some code that Richard Henderson said was invalid.  After
modifying convert_memory_address I had a couple of failures that
involved inlining memcpy and friends so I had to modify opcodes.c to fix
those failures by ensuring that the routines return ptr_mode addresses
and not Pmode ones.

Also, I was originally going to skip the permutation of the PLUS and the
conversion when POINTERS_EXTEND_UNSIGNED < 0, but I found that I could
not compile gcc.c-torture/compile/20010107-1.c in 32 bit mode without
doing this optimization.  The compiler would die with:

20010107-1.c:6: error: unrecognizable insn:
(insn 36 29 37 0 00000000 (set (reg:DI 15 r15)
        (plus:DI (reg:DI 1 r1)
            (const:DI (reg:DI 340)))) -1 (nil)
    (nil))

Since nothing failed when I did do this optimization in
convert_memory_address I enabled it for IA64 HP-UX.

Steve Ellcey
sje@cup.hp.com



2002-07-22  Steve Ellcey  <sje@cup.hp.com>

	* gcc/explow.c (convert_memory_address): Fix conversion of CONSTs.
	Fix permutation of conversion and plus/mult.
	* gcc/builtins.c (expand_builtin_memcpy) Ensure return pointer is
	ptr_mode and not Pmode when POINTERS_EXTEND_UNSIGNED is defined.
	(expand_builtin_strncpy) Ditto.
	(expand_builtin_memset) Ditto.



*** gcc.orig/gcc/explow.c	Mon Jul 22 08:24:46 2002
--- gcc/gcc/explow.c	Mon Jul 22 11:05:18 2002
*************** convert_memory_address (to_mode, x)
*** 353,358 ****
--- 353,359 ----
  {
    enum machine_mode from_mode = to_mode == ptr_mode ? Pmode : ptr_mode;
    rtx temp;
+   enum rtx_code code;
  
    /* Here we handle some special cases.  If none of them apply, fall through
       to the default case.  */
*************** convert_memory_address (to_mode, x)
*** 360,366 ****
      {
      case CONST_INT:
      case CONST_DOUBLE:
!       return x;
  
      case SUBREG:
        if ((SUBREG_PROMOTED_VAR_P (x) || REG_POINTER (SUBREG_REG (x)))
--- 361,378 ----
      {
      case CONST_INT:
      case CONST_DOUBLE:
!       if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode))
! 	code = TRUNCATE;
!       else if (POINTERS_EXTEND_UNSIGNED < 0)
! 	break;
!       else if (POINTERS_EXTEND_UNSIGNED > 0)
! 	code = ZERO_EXTEND;
!       else
! 	code = SIGN_EXTEND;
!       temp = simplify_unary_operation (code, to_mode, x, from_mode);
!       if (temp)
! 	return temp;
!       break;
  
      case SUBREG:
        if ((SUBREG_PROMOTED_VAR_P (x) || REG_POINTER (SUBREG_REG (x)))
*************** convert_memory_address (to_mode, x)
*** 389,405 ****
  
      case PLUS:
      case MULT:
!       /* For addition the second operand is a small constant, we can safely
! 	 permute the conversion and addition operation.  We can always safely
! 	 permute them if we are making the address narrower.  In addition,
! 	 always permute the operations if this is a constant.  */
!       if ((GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
! 	      || (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT
! 		  && (INTVAL (XEXP (x, 1)) + 20000 < 40000
! 		      || CONSTANT_P (XEXP (x, 0))))))
  	return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
  			       convert_memory_address (to_mode, XEXP (x, 0)),
! 			       convert_memory_address (to_mode, XEXP (x, 1)));
        break;
  
      default:
--- 401,417 ----
  
      case PLUS:
      case MULT:
!       /* For addition we can safely permute the conversion and addition
! 	 operation if one operand is a constant and converting the constant
! 	 does not change it.  We can always safely permute them if we are
! 	 making the address narrower.  */
!       if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
! 	  || (GET_CODE (x) == PLUS
! 	      && GET_CODE (XEXP (x, 1)) == CONST_INT
! 	      && XEXP (x, 1) == convert_memory_address (to_mode, XEXP (x, 1))))
  	return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
  			       convert_memory_address (to_mode, XEXP (x, 0)),
! 			       XEXP (x, 1));
        break;
  
      default:
*** gcc.orig/gcc/builtins.c	Mon Jul 22 08:24:50 2002
--- gcc/gcc/builtins.c	Mon Jul 22 09:34:57 2002
*************** expand_builtin_memcpy (arglist, target, 
*** 1981,1987 ****
  	  store_by_pieces (dest_mem, INTVAL (len_rtx),
  			   builtin_memcpy_read_str,
  			   (PTR) src_str, dest_align);
! 	  return force_operand (XEXP (dest_mem, 0), NULL_RTX);
  	}
  
        src_mem = get_memory_rtx (src);
--- 1981,1992 ----
  	  store_by_pieces (dest_mem, INTVAL (len_rtx),
  			   builtin_memcpy_read_str,
  			   (PTR) src_str, dest_align);
! 	  dest_mem = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_mem) != ptr_mode)
! 	    dest_mem = convert_memory_address (ptr_mode, dest_mem);
! #endif
! 	  return dest_mem;
  	}
  
        src_mem = get_memory_rtx (src);
*************** expand_builtin_memcpy (arglist, target, 
*** 1991,1997 ****
        dest_addr = emit_block_move (dest_mem, src_mem, len_rtx);
  
        if (dest_addr == 0)
! 	dest_addr = force_operand (XEXP (dest_mem, 0), NULL_RTX);
  
        return dest_addr;
      }
--- 1996,2008 ----
        dest_addr = emit_block_move (dest_mem, src_mem, len_rtx);
  
        if (dest_addr == 0)
! 	{
! 	  dest_addr = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_addr) != ptr_mode)
! 	    dest_addr = convert_memory_address (ptr_mode, dest_addr);
! #endif
! 	}
  
        return dest_addr;
      }
*************** expand_builtin_strncpy (arglist, target,
*** 2107,2113 ****
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_strncpy_read_str,
  			   (PTR) p, dest_align);
! 	  return force_operand (XEXP (dest_mem, 0), NULL_RTX);
  	}
  
        /* OK transform into builtin memcpy.  */
--- 2118,2129 ----
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_strncpy_read_str,
  			   (PTR) p, dest_align);
! 	  dest_mem = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_mem) != ptr_mode)
! 	    dest_mem = convert_memory_address (ptr_mode, dest_mem);
! #endif
! 	  return dest_mem;
  	}
  
        /* OK transform into builtin memcpy.  */
*************** expand_builtin_memset (exp, target, mode
*** 2232,2238 ****
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_memset_gen_str,
  			   (PTR)val_rtx, dest_align);
! 	  return force_operand (XEXP (dest_mem, 0), NULL_RTX);
  	}
  
        if (target_char_cast (val, &c))
--- 2248,2259 ----
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_memset_gen_str,
  			   (PTR)val_rtx, dest_align);
! 	  dest_mem = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_mem) != ptr_mode)
! 	    dest_mem = convert_memory_address (ptr_mode, dest_mem);
! #endif
! 	  return dest_mem;
  	}
  
        if (target_char_cast (val, &c))
*************** expand_builtin_memset (exp, target, mode
*** 2251,2257 ****
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_memset_read_str,
  			   (PTR) &c, dest_align);
! 	  return force_operand (XEXP (dest_mem, 0), NULL_RTX);
  	}
  
        len_rtx = expand_expr (len, NULL_RTX, VOIDmode, 0);
--- 2272,2283 ----
  	  store_by_pieces (dest_mem, tree_low_cst (len, 1),
  			   builtin_memset_read_str,
  			   (PTR) &c, dest_align);
! 	  dest_mem = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_mem) != ptr_mode)
! 	    dest_mem = convert_memory_address (ptr_mode, dest_mem);
! #endif
! 	  return dest_mem;
  	}
  
        len_rtx = expand_expr (len, NULL_RTX, VOIDmode, 0);
*************** expand_builtin_memset (exp, target, mode
*** 2261,2267 ****
        dest_addr = clear_storage (dest_mem, len_rtx);
  
        if (dest_addr == 0)
! 	dest_addr = force_operand (XEXP (dest_mem, 0), NULL_RTX);
  
        return dest_addr;
      }
--- 2287,2299 ----
        dest_addr = clear_storage (dest_mem, len_rtx);
  
        if (dest_addr == 0)
! 	{
! 	  dest_addr = force_operand (XEXP (dest_mem, 0), NULL_RTX);
! #ifdef POINTERS_EXTEND_UNSIGNED
! 	  if (GET_MODE (dest_addr) != ptr_mode)
! 	    dest_addr = convert_memory_address (ptr_mode, dest_addr);
! #endif
! 	}
  
        return dest_addr;
      }

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Steve Ellcey]

> How, exactly, do things go wrong?
> 
> Is it that -2000000000L is signed, but unsigned or ptr-extending
> pointers frob the high bits in a bad way?  If so, the proper
> solution is to remove that range check, and
> 
>   (1) if sign- or zero-extending pointers, verify that the constant
>       is the same after conversion.
>   (2) if ptr-extending, don't ever re-associate the operations.
>       This shouldn't hurt ia64, since we don't have much in the
>       way of special address arithmetic that this reassociation
>       is hoping to cater to.

The problem on IA64 is that the pointer extending instruction (addp4)
copies bits 30 and 31 (the two high order bits) into bits 61 and 62.  If
we take a 32 bit address and add (or subtract) a constant that is large
enough to result in a value that is different in bits 30 or 31 from the
original value and then extend it with addp4 and then subtract (or add)
the original constant back in to the 64 bit address we get the wrong
result, different then doing both the add and subtract in 32 bit mode
and then extending the address.  As long as the (32 bit) address
manipulation doesn't affect bits 30 and 31, everything is fine.

Since this seems specific to IA64 and addp4 I think the simplest
solution is to turn off the optimization if using a ptr_extend
instruction (and not zero or sign extension).  I'd like to leave the
sign-extend/zero-extend case alone since I don't have a platform that
does either of those types of extension.



> Incidentally, the CONST_INT/CONST_DOUBLE case looks wrong.  We
> should be adjusting the constant according to POINTERS_EXTEND_UNSIGNED
> from from_mode to to_mode.  Really that means
> 
>   case CONST_INT: 
>   case CONST_DOUBLE:
>     if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode))
>       code = TRUNCATE;
>     else if (POINTERS_EXTEND_UNSIGNED)
>       code = ZERO_EXTEND;
>     else
>       code = SIGN_EXTEND;
>     return simplify_unary_operation (code, to_mode, x, from_mode);

I will include this as well as the change to not do the PLUS
optimization when using ptr_extend and submit a new patch after testing.

simplify_unary_operation says it returns zero if it can't do any
simplification.  We shouldn't ever hit that case here, but I was
wondering if it was considered a good idea (by GCC coding standards) to
test the result anyway and abort if it did return zero.

Steve Ellcey
sje@cup.hp.com

Re: POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Richard Henderson]

On Tue, Jul 16, 2002 at 02:35:38PM -0700, Steve Ellcey wrote:
> The comments under 'case PLUS' and 'case MULT' in convert_memory_address
> say the optimization for PLUS is safe for small constants but the test
> is:
> 
> 	 INTVAL (XEXP (x, 1)) + 20000 < 40000

This is completely bogus.  Either any constant is valid or no
constant is valid.

> And we do the optimization and wind up generating bad code.

How, exactly, do things go wrong?

Is it that -2000000000L is signed, but unsigned or ptr-extending
pointers frob the high bits in a bad way?  If so, the proper
solution is to remove that range check, and

  (1) if sign- or zero-extending pointers, verify that the constant
      is the same after conversion.
  (2) if ptr-extending, don't ever re-associate the operations.
      This shouldn't hurt ia64, since we don't have much in the
      way of special address arithmetic that this reassociation
      is hoping to cater to.

Incidentally, the CONST_INT/CONST_DOUBLE case looks wrong.  We
should be adjusting the constant according to POINTERS_EXTEND_UNSIGNED
from from_mode to to_mode.  Really that means

  case CONST_INT: 
  case CONST_DOUBLE:
    if (GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode))
      code = TRUNCATE;
    else if (POINTERS_EXTEND_UNSIGNED)
      code = ZERO_EXTEND;
    else
      code = SIGN_EXTEND;
    return simplify_unary_operation (code, to_mode, x, from_mode);


r~

POINTERS_EXTEND_UNSIGNED fix (found on IA64 HP-UX)

[Steve Ellcey]

This is a patch to fix the failure of gcc.c-torture/execute/960321-1.c
on the HP-UX IA64 platform in ILP32 mode.  It could affect other platforms
that set POINTERS_EXTEND_UNSIGNED and use convert_memory_address.

The comments under 'case PLUS' and 'case MULT' in convert_memory_address
say the optimization for PLUS is safe for small constants but the test
is:

	 INTVAL (XEXP (x, 1)) + 20000 < 40000

which is true for all negative value since this is a signed comparision.
In 960321-1 we wind up calling convert_memory_address with:

(plus:SI (reg/v:SI 341)
    (const_int -2000000000 [0xffffffff88ca6c00]))

And we do the optimization and wind up generating bad code.  This patch
changes the code to specifically check for values between -20000 and
+20000 which is what I believe the code was intended to do.

Steve Ellcey
sje@cup.hp.com



2002-07-16  Steve Ellcey  <sje@cup.hp.com>

	* gcc/explow.c (convert_memory_address): Fix test for small number.



*** gcc.orig/gcc/explow.c	Tue Jul 16 14:06:15 2002
--- gcc/gcc/explow.c	Tue Jul 16 14:06:36 2002
*************** convert_memory_address (to_mode, x)
*** 395,401 ****
  	 always permute the operations if this is a constant.  */
        if ((GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
  	      || (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT
! 		  && (INTVAL (XEXP (x, 1)) + 20000 < 40000
  		      || CONSTANT_P (XEXP (x, 0))))))
  	return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
  			       convert_memory_address (to_mode, XEXP (x, 0)),
--- 395,402 ----
  	 always permute the operations if this is a constant.  */
        if ((GET_MODE_SIZE (to_mode) < GET_MODE_SIZE (from_mode)
  	      || (GET_CODE (x) == PLUS && GET_CODE (XEXP (x, 1)) == CONST_INT
! 		  && ((INTVAL (XEXP (x, 1)) > -20000
! 		       && INTVAL (XEXP (x, 1)) < 20000)
  		      || CONSTANT_P (XEXP (x, 0))))))
  	return gen_rtx_fmt_ee (GET_CODE (x), to_mode,
  			       convert_memory_address (to_mode, XEXP (x, 0)),