remove wrong code in immed_double_const

remove wrong code in immed_double_const

[Mike Stump]

This removes some wrong code.

Ok?

Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c      (revision 184563)
+++ gcc/emit-rtl.c      (working copy)
@@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
        return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */

Re: remove wrong code in immed_double_const

[Steven Bosscher]

On Fri, Mar 16, 2012 at 10:54 PM, Mike Stump <mikestump@comcast.net> wrote:
> This removes some wrong code.
>
> Ok?

ChangeLog is missing. Tested how?
And why is this wrong anyway?

Ciao!
Steven



> Index: gcc/emit-rtl.c
> ===================================================================
> --- gcc/emit-rtl.c      (revision 184563)
> +++ gcc/emit-rtl.c      (working copy)
> @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>
>       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>        return gen_int_mode (i0, mode);
> -
> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>     }
>
>   /* If this integer fits in one word, return a CONST_INT.  */
>

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 16, 2012, at 3:03 PM, Steven Bosscher wrote:
> On Fri, Mar 16, 2012 at 10:54 PM, Mike Stump <mikestump@comcast.net> wrote:
>> This removes some wrong code.
>> 
>> Ok?
> 
> ChangeLog is missing.

	* emit-rtl.c (immed_double_const): Remove bogus assert.

> Tested how?

Compiles a user program for my port.

> And why is this wrong anyway?

It is wrong because it can assert.  The code can generate a constant for every inbound value that it asserts for, so trivially, it is wrong.  If it were correct, it would not assert for values it can handle.  For it to be correct, there would have to exist a value for which the code fails.  The value that failed for me was 0, not a terribly uncommon value.

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> This removes some wrong code.
>
> Ok?
>
> Index: gcc/emit-rtl.c
> ===================================================================
> --- gcc/emit-rtl.c      (revision 184563)
> +++ gcc/emit-rtl.c      (working copy)
> @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>  
>        if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>         return gen_int_mode (i0, mode);
> -
> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>      }
>  
>    /* If this integer fits in one word, return a CONST_INT.  */

Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and
2 * HOST_BITS_PER_WIDE_INT?  (I.e. a partial one?)  If so, I can see an
argument for changing the "==" to "<=", although we'd need to think
carefully about what CONST_DOUBLE means in that case.  (Endianness, etc.)

Or is this because you have an integer mode wider than
2*OST_BITS_PER_WIDE_INT?  We currently only support constant integer
widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly
triggering if we try to build a wider constant.  Obviously it'd be
nice if we supported arbitrary widths, e.g. by replacing CONST_INT and
CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const
with some kind of nary builder, etc.).  It won't be easy though.

Removing the assert seems like papering over the problem.

FWIW, here's another case where this came up:

    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html

Richard

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 17, 2012, at 12:37 AM, Richard Sandiford wrote:
> Mike Stump <mikestump@comcast.net> writes:
>> This removes some wrong code.
>> 
>> Ok?
>> 
>> Index: gcc/emit-rtl.c
>> ===================================================================
>> --- gcc/emit-rtl.c      (revision 184563)
>> +++ gcc/emit-rtl.c      (working copy)
>> @@ -540,8 +540,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>> 
>>       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>>        return gen_int_mode (i0, mode);
>> -
>> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>>     }
>> 
>>   /* If this integer fits in one word, return a CONST_INT.  */
> 
> Is this because you have an integer mode between HOST_BITS_PER_WIDE_INT and
> 2 * HOST_BITS_PER_WIDE_INT?

Yes, I have those, but, that wasn't the testcase I had in mind.

> Or is this because you have an integer mode wider than
> 2*OST_BITS_PER_WIDE_INT?

Yes.

> We currently only support constant integer
> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly
> triggering if we try to build a wider constant.

Can you give me a concrete example of what will fail with the constant 0 generated by:

	return GEN_INT (i0);

when the mode is 2*HOST_BITS_PER_WIDE_INT?  When the mode is larger than this?

If you cannot, can you refer me to documentation where this is discussed?  If not, how about code?

What I am seeing is that it works and generates correct code without the assert.  My contention is that any code that fails to work, is buggy, and should be fixed, and once fixed, then there is no code that doesn't work, and hence the assert is wrong.  If you want to argue that the code that fails, isn't buggy, go ahead, I'd love to hear it.

See, we already shorten the width of wide constants and expect that to work.  This is just another instance of shortening.  Now, just to see what lurks in there, I when through 100% of the uses of CONST_DOUBLE in *.c to get a feel for what might go wrong, the code is as benign as I expected, every instance I saw, looked reasonably safe.  It doesn't get any better than this.

> Obviously it'd be nice

Yes, but that is quite a lot of work.  It also happens to be orthogonal to the immediate issue at hand.

> if we supported arbitrary widths, e.g. by replacing CONST_INT and
> CONST_DOUBLE with a single n-HOST_WIDE_INT rtx (and immed_double_const
> with some kind of nary builder, etc.).  It won't be easy though.
> 
> Removing the assert seems like papering over the problem.

Do you have an example of a failure?  Hint, without a failure, there is no bug, I cannot fix a bug, when there is no bug.

> FWIW, here's another case where this came up:
> 
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html

Yes, and surprisingly, or not it is the exact same case as I can into.  Notice nowhere in that bug or thread, is _any_ detailed discussed as to why that would be a bad fix.

So, I'm looking for approval, or a concrete reason why it is a bad idea.

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
>> We currently only support constant integer
>> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly
>> triggering if we try to build a wider constant.
>
> Can you give me a concrete example of what will fail with the constant
> 0 generated by:
>
> 	return GEN_INT (i0);
>
> when the mode is 2*HOST_BITS_PER_WIDE_INT?  When the mode is larger than this?

I think the assert is more saying that things have already gone wrong
if we have reached this point.  immed_double_const is only designed
to handle two HOST_WIDE_INTs, so what exactly is the caller trying to do?
E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs?  If we're
going to remove the assert, we need to define stuff like that.

All ones is a pretty common constant, so if I was going to guess,
I'd have expected we'd sign extend, just like we do for CONST_INT.
So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1).
But if we do that, we need to define the high HOST_WIDE_INT of a
CONST_DOUBLE to be sign-extended too.  And at the moment we don't;
the CONST_DOUBLE case of simplify_immed_subreg says:

	      /* It shouldn't matter what's done here, so fill it with
		 zero.  */
	      for (; i < elem_bitsize; i += value_bit)
		*vp++ = 0;

Compare with the CONST_INT case:

	  /* CONST_INTs are always logically sign-extended.  */
	  for (; i < elem_bitsize; i += value_bit)
	    *vp++ = INTVAL (el) < 0 ? -1 : 0;

So...

> See, we already shorten the width of wide constants and expect that to
> work.  This is just another instance of shortening.  Now, just to see
> what lurks in there, I when through 100% of the uses of CONST_DOUBLE
> in *.c to get a feel for what might go wrong, the code is as benign as
> I expected, every instance I saw, looked reasonably safe.  It doesn't
> get any better than this.

...I'm not sure about.

From a quick grep, it looks like the case where I saw the immed_double_const
assert triggering -- the expansion of INTEGER_CST -- is the only case
where it could trigger.  So I suppose the question shifts to the tree level.

Are callers to build_int_cst_wide and double_int_to_tree already
expected to check that constants wider than a double_int would not
be truncated?  If so, then great.  And if so, what are the rules
there regarding sign vs. zero extension beyond the high HOST_WIDE_INT?

>> FWIW, here's another case where this came up:
>> 
>>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html
>
> Yes, and surprisingly, or not it is the exact same case as I can into.

I'd expect it to be a slightly different case.  The original testcase was
a union constructor that was unnecessarily initialised to zero.  That has
since been fixed.

Richard

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 18, 2012, at 3:16 AM, Richard Sandiford wrote:
> Mike Stump <mikestump@comcast.net> writes:
>>> We currently only support constant integer
>>> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly
>>> triggering if we try to build a wider constant.
>> 
>> Can you give me a concrete example of what will fail with the constant
>> 0 generated by:
>> 
>> 	return GEN_INT (i0);
>> 
>> when the mode is 2*HOST_BITS_PER_WIDE_INT?  When the mode is larger than this?
> 
> I think the assert is more saying that things have already gone wrong

You apparently don't get it.  Let me more forcefully state my position.  No, things have not gone wrong.  Let me repeat myself, again, 0 is perfectly representable in 0 bits, by induction it is perfectly representable in n+1 bits without loss.  By induction, every integral size greater than 0 is also perfectly able to represent 0.

If they had gone wrong, the testcase would not work, the code would not compile and I would not get valid code.

> if we have reached this point.  immed_double_const is only designed
> to handle two HOST_WIDE_INTs,

0 fits into two HOST_WIDE_INTs.

> so what exactly is the caller trying to do?

Put 0 into a CONST_INT, with the result mode VOIDmode.

> E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs?

Irrelevant.  0 has the same value, 0 or sign extended, honest.

> If we're going to remove the assert, we need to define stuff like that.

Orthogonal.  The rest of the compiler defines what happens, it either is inconsistent, in which case it is by fiat, undefined, or it is consistent, in which case that consistency defines it.  The compiler is free to document this in a nice way, or do, what is usually done, which is to assume everybody just knows what it does.  Anyway, my point is, this routine doesn't define the data structure, and is _completely_ orthogonal to your concern.  It doesn't matter if it zero extends or sign extends or is inconsistent, has bugs, doesn't have bugs, is documented, or isn't documented.  In every single one of these cases, the code in the routine I am fixing, doesn't change.  That is _why_ it is orthogonal.  If it weren't, you'd be able to state a value for which is mattered.  You can't, which is why you are wrong.  If you think you are not wrong, please state a value for which it matters how it is defined.

> All ones is a pretty common constant, so if I was going to guess,
> I'd have expected we'd sign extend,

Idle and irrelevant speculation.  Let's not go down that path.

> So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1).
> But if we do that, we need to define the high HOST_WIDE_INT of a
> CONST_DOUBLE to be sign-extended too.  And at the moment we don't;

Though this is orthogonal to the patch under consideration, I'd agree, defining the values as being sign extending seems reasonable to me.

> the CONST_DOUBLE case of simplify_immed_subreg says:
> 
> 	      /* It shouldn't matter what's done here, so fill it with
> 		 zero.  */
> 	      for (; i < elem_bitsize; i += value_bit)
> 		*vp++ = 0;

And this would would have to be fixed, if _you_ wanted to define it as sign extending.  Given this code, by fiat, it is defined to either be inconsistent or to be zero extending, presently.

> From a quick grep, it looks like the case where I saw the immed_double_const

> assert triggering -- the expansion of INTEGER_CST -- is the only case
> where it could trigger.  So I suppose the question shifts to the tree level.

Again, irrelevant, let's not go there.

I do not propose to define the data structure in my patch, nor to change the definition of the data structure.  I merely propose to fix an obvious and trivial bug.

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
>> If we're going to remove the assert, we need to define stuff like
>> that.
>
> Orthogonal.  The rest of the compiler defines what happens, it either
> is inconsistent, in which case it is by fiat, undefined, or it is
> consistent, in which case that consistency defines it.  The compiler
> is free to document this in a nice way, or do, what is usually done,
> which is to assume everybody just knows what it does.  Anyway, my
> point is, this routine doesn't define the data structure, and is
> _completely_ orthogonal to your concern.  It doesn't matter if it zero
> extends or sign extends or is inconsistent, has bugs, doesn't have
> bugs, is documented, or isn't documented.  In every single one of
> these cases, the code in the routine I am fixing, doesn't change.
> That is _why_ it is orthogonal.  If it weren't, you'd be able to state
> a value for which is mattered.  You can't, which is why you are wrong.
> If you think you are not wrong, please state a value for which it
> matters how it is defined.

It isn't orthoganal.  immed_double_const and CONST_DOUBLE are currently
only defined for 2 HOST_WIDE_INTs.  So, as good functions do,
immed_double_const asserts that it is not being used out of spec.
That's why the code I quoted said the filler bits for CONST_DOUBLE
shouldn't matter; this assert is supposed to make sure that we never
generate CONST_DOUBLEs like that.  (Personally I'd have preferred it
if simplify_immed_subreg asserted instead of filling with zeros.)

You want to remove that restriction on immed_double_const and CONST_DOUBLE.
That is, you want to change their spec.  We should only do that if we define
what the new semantics are.

Richard

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote:
> Mike Stump <mikestump@comcast.net> writes:
>>> If we're going to remove the assert, we need to define stuff like
>>> that.
>> 
>> Orthogonal.  The rest of the compiler defines what happens, it either
>> is inconsistent, in which case it is by fiat, undefined, or it is
>> consistent, in which case that consistency defines it.  The compiler
>> is free to document this in a nice way, or do, what is usually done,
>> which is to assume everybody just knows what it does.  Anyway, my
>> point is, this routine doesn't define the data structure, and is
>> _completely_ orthogonal to your concern.  It doesn't matter if it zero
>> extends or sign extends or is inconsistent, has bugs, doesn't have
>> bugs, is documented, or isn't documented.  In every single one of
>> these cases, the code in the routine I am fixing, doesn't change.
>> That is _why_ it is orthogonal.  If it weren't, you'd be able to state
>> a value for which is mattered.  You can't, which is why you are wrong.
>> If you think you are not wrong, please state a value for which it
>> matters how it is defined.
> 
> immed_double_const and CONST_DOUBLE are currently
> only defined for 2 HOST_WIDE_INTs.

I don't happen to share your view.  The routine is defined by documentation.  The documentation might exist in a .texi file, in this case there is no texi file for immed_double_const I don't think, next up, it is defined by the comments before the routine.  In this case, it isn't so defined.

The current definition reads:

/* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair                                                                                        
   of ints: I0 is the low-order word and I1 is the high-order word.                                                                                          
   Do not use this routine for non-integer modes; convert to                                                                                                 
   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */

which, is is fine, and I don't _want_ to change that definition of the routine.  I can't fix it, because it isn't broken.  If it were, you would be able to state a case where the new code behaves in a manor inconsistent with the definition, since there is none you cannot state one, and this is _why_ you have failed to state such a case.  If you disagree, please state the case.

Now, if you review comment is, could you please update the comments in the routine, I would just say, oh, sure:

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 184563)
+++ emit-rtl.c	(working copy)
@@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+	(i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
@@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */


Sorry I missed it.  Now, on to CONST_DOUBLE.  It does appear in a texi file:


@findex const_double
@item (const_double:@var{m} @var{i0} @var{i1} @dots{})
Represents either a floating-point constant of mode @var{m} or an
integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
bits but small enough to fit within twice that number of bits (GCC
does not provide a mechanism to represent even larger constants).  In
the latter case, @var{m} will be @code{VOIDmode}.

@findex CONST_DOUBLE_LOW
If @var{m} is @code{VOIDmode}, the bits of the value are stored in
@var{i0} and @var{i1}.  @var{i0} is customarily accessed with the macro
@code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}.


Here again, I don't want to change the definition.  The current definition applies and I am merely making the code conform to it.  It says that CONST_DOUBLE is used when the _value_ of the constant is too large to fit into HOST_BITS_PER_WIDE_INT bits.

So, if you disagree with me, you will necessarily have to quote the definition you are using, explain what the words mean to you _and_ state a specific case in which the code post modification doesn't not conform with the existing definition.  You have failed yet again to do that.


> So, as good functions do, immed_double_const asserts that it is not being used out of spec.

This does not follow from the definition.  0 is a value that fits into HOST_BITS_PER_WIDE_INT bits.  It is representable in 0 bits.  HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits.

> You want to remove that restriction on immed_double_const and CONST_DOUBLE.
> That is, you want to change their spec.  We should only do that if we define
> what the new semantics are.

You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file.

Re: remove wrong code in immed_double_const

[Richard Guenther]

On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote:
>> Mike Stump <mikestump@comcast.net> writes:
>>>> If we're going to remove the assert, we need to define stuff like
>>>> that.
>>>
>>> Orthogonal.  The rest of the compiler defines what happens, it either
>>> is inconsistent, in which case it is by fiat, undefined, or it is
>>> consistent, in which case that consistency defines it.  The compiler
>>> is free to document this in a nice way, or do, what is usually done,
>>> which is to assume everybody just knows what it does.  Anyway, my
>>> point is, this routine doesn't define the data structure, and is
>>> _completely_ orthogonal to your concern.  It doesn't matter if it zero
>>> extends or sign extends or is inconsistent, has bugs, doesn't have
>>> bugs, is documented, or isn't documented.  In every single one of
>>> these cases, the code in the routine I am fixing, doesn't change.
>>> That is _why_ it is orthogonal.  If it weren't, you'd be able to state
>>> a value for which is mattered.  You can't, which is why you are wrong.
>>> If you think you are not wrong, please state a value for which it
>>> matters how it is defined.
>>
>> immed_double_const and CONST_DOUBLE are currently
>> only defined for 2 HOST_WIDE_INTs.
>
> I don't happen to share your view.  The routine is defined by documentation.  The documentation might exist in a .texi file, in this case there is no texi file for immed_double_const I don't think, next up, it is defined by the comments before the routine.  In this case, it isn't so defined.
>
> The current definition reads:
>
> /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
>   of ints: I0 is the low-order word and I1 is the high-order word.
>   Do not use this routine for non-integer modes; convert to
>   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
>
> which, is is fine, and I don't _want_ to change that definition of the routine.  I can't fix it, because it isn't broken.  If it were, you would be able to state a case where the new code behaves in a manor inconsistent with the definition, since there is none you cannot state one, and this is _why_ you have failed to state such a case.  If you disagree, please state the case.
>
> Now, if you review comment is, could you please update the comments in the routine, I would just say, oh, sure:
>
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c  (revision 184563)
> +++ emit-rtl.c  (working copy)
> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
>
>      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>        gen_int_mode.
> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
> -       the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
> -       from copies of the sign bit, and sign of i0 and i1 are the same),  then
> -       we return a CONST_INT for i0.
> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
> +       (i.e., i1 consists only from copies of the sign bit, and sign
> +       of i0 and i1 are the same), then we return a CONST_INT for i0.
>      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
>   if (mode != VOIDmode)
>     {
> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>
>       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>        return gen_int_mode (i0, mode);
> -
> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>     }
>
>   /* If this integer fits in one word, return a CONST_INT.  */
>
>
> Sorry I missed it.  Now, on to CONST_DOUBLE.  It does appear in a texi file:
>
>
> @findex const_double
> @item (const_double:@var{m} @var{i0} @var{i1} @dots{})
> Represents either a floating-point constant of mode @var{m} or an
> integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
> bits but small enough to fit within twice that number of bits (GCC
> does not provide a mechanism to represent even larger constants).  In
> the latter case, @var{m} will be @code{VOIDmode}.
>
> @findex CONST_DOUBLE_LOW
> If @var{m} is @code{VOIDmode}, the bits of the value are stored in
> @var{i0} and @var{i1}.  @var{i0} is customarily accessed with the macro
> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}.
>
>
> Here again, I don't want to change the definition.  The current definition applies and I am merely making the code conform to it.  It says that CONST_DOUBLE is used when the _value_ of the constant is too large to fit into HOST_BITS_PER_WIDE_INT bits.
>
> So, if you disagree with me, you will necessarily have to quote the definition you are using, explain what the words mean to you _and_ state a specific case in which the code post modification doesn't not conform with the existing definition.  You have failed yet again to do that.
>
>
>> So, as good functions do, immed_double_const asserts that it is not being used out of spec.
>
> This does not follow from the definition.  0 is a value that fits into HOST_BITS_PER_WIDE_INT bits.  It is representable in 0 bits.  HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits.
>
>> You want to remove that restriction on immed_double_const and CONST_DOUBLE.
>> That is, you want to change their spec.  We should only do that if we define
>> what the new semantics are.
>
> You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file.

Btw, I agree with Mike here (quite obvious if you followed the old
e-mail thread).
But as there is some disagreement here I leave approval of the patch with the
comment change to someone to break that tie ;)

Thanks,
Richard.

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Richard Guenther <richard.guenther@gmail.com> writes:
> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote:
>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote:
>>> Mike Stump <mikestump@comcast.net> writes:
>>>>> If we're going to remove the assert, we need to define stuff like
>>>>> that.
>>>>
>>>> Orthogonal.  The rest of the compiler defines what happens, it either
>>>> is inconsistent, in which case it is by fiat, undefined, or it is
>>>> consistent, in which case that consistency defines it.  The compiler
>>>> is free to document this in a nice way, or do, what is usually done,
>>>> which is to assume everybody just knows what it does.  Anyway, my
>>>> point is, this routine doesn't define the data structure, and is
>>>> _completely_ orthogonal to your concern.  It doesn't matter if it zero
>>>> extends or sign extends or is inconsistent, has bugs, doesn't have
>>>> bugs, is documented, or isn't documented.  In every single one of
>>>> these cases, the code in the routine I am fixing, doesn't change.
>>>> That is _why_ it is orthogonal.  If it weren't, you'd be able to state
>>>> a value for which is mattered.  You can't, which is why you are wrong.
>>>> If you think you are not wrong, please state a value for which it
>>>> matters how it is defined.
>>>
>>> immed_double_const and CONST_DOUBLE are currently
>>> only defined for 2 HOST_WIDE_INTs.
>>
>> I don't happen to share your view.  The routine is defined by documentation.  The documentation might exist in a .texi file, in this case there is no texi file for immed_double_const I don't think, next up, it is defined by the comments before the routine.  In this case, it isn't so defined.
>>
>> The current definition reads:
>>
>> /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
>>   of ints: I0 is the low-order word and I1 is the high-order word.
>>   Do not use this routine for non-integer modes; convert to
>>   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
>>
>> which, is is fine, and I don't _want_ to change that definition of the routine.  I can't fix it, because it isn't broken.  If it were, you would be able to state a case where the new code behaves in a manor inconsistent with the definition, since there is none you cannot state one, and this is _why_ you have failed to state such a case.  If you disagree, please state the case.
>>
>> Now, if you review comment is, could you please update the comments in the routine, I would just say, oh, sure:
>>
>> Index: emit-rtl.c
>> ===================================================================
>> --- emit-rtl.c  (revision 184563)
>> +++ emit-rtl.c  (working copy)
>> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
>>
>>      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>>        gen_int_mode.
>> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
>> -       the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
>> -       from copies of the sign bit, and sign of i0 and i1 are the same),  then
>> -       we return a CONST_INT for i0.
>> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
>> +       (i.e., i1 consists only from copies of the sign bit, and sign
>> +       of i0 and i1 are the same), then we return a CONST_INT for i0.
>>      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
>>   if (mode != VOIDmode)
>>     {
>> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>>
>>       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>>        return gen_int_mode (i0, mode);
>> -
>> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>>     }
>>
>>   /* If this integer fits in one word, return a CONST_INT.  */
>>
>>
>> Sorry I missed it.  Now, on to CONST_DOUBLE.  It does appear in a texi file:
>>
>>
>> @findex const_double
>> @item (const_double:@var{m} @var{i0} @var{i1} @dots{})
>> Represents either a floating-point constant of mode @var{m} or an
>> integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
>> bits but small enough to fit within twice that number of bits (GCC
>> does not provide a mechanism to represent even larger constants).  In
>> the latter case, @var{m} will be @code{VOIDmode}.
>>
>> @findex CONST_DOUBLE_LOW
>> If @var{m} is @code{VOIDmode}, the bits of the value are stored in
>> @var{i0} and @var{i1}.  @var{i0} is customarily accessed with the macro
>> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}.
>>
>>
>> Here again, I don't want to change the definition.  The current definition applies and I am merely making the code conform to it.  It says that CONST_DOUBLE is used when the _value_ of the constant is too large to fit into HOST_BITS_PER_WIDE_INT bits.
>>
>> So, if you disagree with me, you will necessarily have to quote the definition you are using, explain what the words mean to you _and_ state a specific case in which the code post modification doesn't not conform with the existing definition.  You have failed yet again to do that.
>>
>>
>>> So, as good functions do, immed_double_const asserts that it is not being used out of spec.
>>
>> This does not follow from the definition.  0 is a value that fits into HOST_BITS_PER_WIDE_INT bits.  It is representable in 0 bits.  HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits.
>>
>>> You want to remove that restriction on immed_double_const and CONST_DOUBLE.
>>> That is, you want to change their spec.  We should only do that if we define
>>> what the new semantics are.
>>
>> You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file.
>
> Btw, I agree with Mike here (quite obvious if you followed the old
> e-mail thread).

I've no objection to moving the assert down to after the GEN_INT.
But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing.
(That is, if we remove the assert altogether, we effectively treat the
number as sign-extended if it happens to fit in a CONST_INT, and
zero-extended otherwise.  That kind of inconsistency seems wrong,
and could turn what is now an ICE into a wrong code bug.)

> But as there is some disagreement here I leave approval of the patch with the
> comment change to someone to break that tie ;)

No need for that.  Clearly it's just me :-)  Please go ahead and approve
whatever you think is right.

Richard

Re: remove wrong code in immed_double_const

[Richard Guenther]

On Tue, Mar 20, 2012 at 11:49 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>> On Tue, Mar 20, 2012 at 12:31 AM, Mike Stump <mikestump@comcast.net> wrote:
>>> On Mar 19, 2012, at 2:44 PM, Richard Sandiford wrote:
>>>> Mike Stump <mikestump@comcast.net> writes:
>>>>>> If we're going to remove the assert, we need to define stuff like
>>>>>> that.
>>>>>
>>>>> Orthogonal.  The rest of the compiler defines what happens, it either
>>>>> is inconsistent, in which case it is by fiat, undefined, or it is
>>>>> consistent, in which case that consistency defines it.  The compiler
>>>>> is free to document this in a nice way, or do, what is usually done,
>>>>> which is to assume everybody just knows what it does.  Anyway, my
>>>>> point is, this routine doesn't define the data structure, and is
>>>>> _completely_ orthogonal to your concern.  It doesn't matter if it zero
>>>>> extends or sign extends or is inconsistent, has bugs, doesn't have
>>>>> bugs, is documented, or isn't documented.  In every single one of
>>>>> these cases, the code in the routine I am fixing, doesn't change.
>>>>> That is _why_ it is orthogonal.  If it weren't, you'd be able to state
>>>>> a value for which is mattered.  You can't, which is why you are wrong.
>>>>> If you think you are not wrong, please state a value for which it
>>>>> matters how it is defined.
>>>>
>>>> immed_double_const and CONST_DOUBLE are currently
>>>> only defined for 2 HOST_WIDE_INTs.
>>>
>>> I don't happen to share your view.  The routine is defined by documentation.  The documentation might exist in a .texi file, in this case there is no texi file for immed_double_const I don't think, next up, it is defined by the comments before the routine.  In this case, it isn't so defined.
>>>
>>> The current definition reads:
>>>
>>> /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
>>>   of ints: I0 is the low-order word and I1 is the high-order word.
>>>   Do not use this routine for non-integer modes; convert to
>>>   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
>>>
>>> which, is is fine, and I don't _want_ to change that definition of the routine.  I can't fix it, because it isn't broken.  If it were, you would be able to state a case where the new code behaves in a manor inconsistent with the definition, since there is none you cannot state one, and this is _why_ you have failed to state such a case.  If you disagree, please state the case.
>>>
>>> Now, if you review comment is, could you please update the comments in the routine, I would just say, oh, sure:
>>>
>>> Index: emit-rtl.c
>>> ===================================================================
>>> --- emit-rtl.c  (revision 184563)
>>> +++ emit-rtl.c  (working copy)
>>> @@ -525,10 +525,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
>>>
>>>      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>>>        gen_int_mode.
>>> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
>>> -       the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
>>> -       from copies of the sign bit, and sign of i0 and i1 are the same),  then
>>> -       we return a CONST_INT for i0.
>>> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
>>> +       (i.e., i1 consists only from copies of the sign bit, and sign
>>> +       of i0 and i1 are the same), then we return a CONST_INT for i0.
>>>      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
>>>   if (mode != VOIDmode)
>>>     {
>>> @@ -540,8 +539,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
>>>
>>>       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
>>>        return gen_int_mode (i0, mode);
>>> -
>>> -      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
>>>     }
>>>
>>>   /* If this integer fits in one word, return a CONST_INT.  */
>>>
>>>
>>> Sorry I missed it.  Now, on to CONST_DOUBLE.  It does appear in a texi file:
>>>
>>>
>>> @findex const_double
>>> @item (const_double:@var{m} @var{i0} @var{i1} @dots{})
>>> Represents either a floating-point constant of mode @var{m} or an
>>> integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
>>> bits but small enough to fit within twice that number of bits (GCC
>>> does not provide a mechanism to represent even larger constants).  In
>>> the latter case, @var{m} will be @code{VOIDmode}.
>>>
>>> @findex CONST_DOUBLE_LOW
>>> If @var{m} is @code{VOIDmode}, the bits of the value are stored in
>>> @var{i0} and @var{i1}.  @var{i0} is customarily accessed with the macro
>>> @code{CONST_DOUBLE_LOW} and @var{i1} with @code{CONST_DOUBLE_HIGH}.
>>>
>>>
>>> Here again, I don't want to change the definition.  The current definition applies and I am merely making the code conform to it.  It says that CONST_DOUBLE is used when the _value_ of the constant is too large to fit into HOST_BITS_PER_WIDE_INT bits.
>>>
>>> So, if you disagree with me, you will necessarily have to quote the definition you are using, explain what the words mean to you _and_ state a specific case in which the code post modification doesn't not conform with the existing definition.  You have failed yet again to do that.
>>>
>>>
>>>> So, as good functions do, immed_double_const asserts that it is not being used out of spec.
>>>
>>> This does not follow from the definition.  0 is a value that fits into HOST_BITS_PER_WIDE_INT bits.  It is representable in 0 bits.  HOST_BITS_PER_WIDE_INT is zero or more, and by induction, is representable by HOST_BITS_PER_WIDE_INT bits.
>>>
>>>> You want to remove that restriction on immed_double_const and CONST_DOUBLE.
>>>> That is, you want to change their spec.  We should only do that if we define
>>>> what the new semantics are.
>>>
>>> You're assuming a definition for CONST_DOUBLE that only exists in your mind, instead, please refer to the actual definition in the .texi file.
>>
>> Btw, I agree with Mike here (quite obvious if you followed the old
>> e-mail thread).
>
> I've no objection to moving the assert down to after the GEN_INT.
> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing.
> (That is, if we remove the assert altogether, we effectively treat the
> number as sign-extended if it happens to fit in a CONST_INT, and
> zero-extended otherwise.

Why do we treat it zero-extended otherwise?  Because we use
gen_int_mode for CONST_INTs, which sign-extends?  Yes, if
we'd have a partial integer mode we would not properly truncate/extend
the double-int input.

OTOH all double-ints should come from trees where the constants are
already properly extended (not sure why we forcefully sign-extend
CONST_INTs for modes that are smaller than a HWI).

>  That kind of inconsistency seems wrong,
> and could turn what is now an ICE into a wrong code bug.)
>
>> But as there is some disagreement here I leave approval of the patch with the
>> comment change to someone to break that tie ;)
>
> No need for that.  Clearly it's just me :-)  Please go ahead and approve
> whatever you think is right.

Well, even if I cannot make sense of the assert (and the assert does not
change the above observation of inconsistency wrt extending partial
integer modes) I'm not familiar enough with RTL to be confident to
approve this change ;)

Richard.

> Richard

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Richard Guenther <richard.guenther@gmail.com> writes:
>> I've no objection to moving the assert down to after the GEN_INT.
>> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing.
>> (That is, if we remove the assert altogether, we effectively treat the
>> number as sign-extended if it happens to fit in a CONST_INT, and
>> zero-extended otherwise.
>
> Why do we treat it zero-extended otherwise?  Because we use
> gen_int_mode for CONST_INTs, which sign-extends?

Just to make sure we're not talking past each other, I meant
moving the assert to:

    /* If this integer fits in one word, return a CONST_INT.  */
[A] if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0))
      return GEN_INT (i0);

<---HERE--->

    /* We use VOIDmode for integers.  */
    value = rtx_alloc (CONST_DOUBLE);
    PUT_MODE (value, VOIDmode);

    CONST_DOUBLE_LOW (value) = i0;
    CONST_DOUBLE_HIGH (value) = i1;

    for (i = 2; i < (sizeof CONST_DOUBLE_FORMAT - 1); i++)
      XWINT (value, i) = 0;

    return lookup_const_double (value);

[A] treats i0 and i1 as a sign-extended value.  So if we
removed the assert (or moved it to the suggested place):

    immed_double_const (-1, -1, 4_hwi_mode)

would create -1 in 4_hwi_mode, represented as a CONST_INT.
The three implicit high-order HWIs are -1.  That's fine,
because CONST_INT has long been defined as sign-extending
rather than zero-extending.

But if we fail the [A] test, we go on to create a CONST_DOUBLE.
The problem is that AIUI we have never defined what happens for
CONST_DOUBLE if the mode is wider than 2 HWIs.  Again AIUI,
that's why the assert is there.

This matters because of things like the handling in simplify_immed_subreg
(which, e.g., we use to generate CONST_DOUBLE pool constants, split
constant moves in lower-subreg.c, etc.).  CONST_INT is already
well-defined to be a sign-extended constant, and we handle it correctly:

      switch (GET_CODE (el))
	{
	case CONST_INT:
	  for (i = 0;
	       i < HOST_BITS_PER_WIDE_INT && i < elem_bitsize;
	       i += value_bit)
	    *vp++ = INTVAL (el) >> i;
	  /* CONST_INTs are always logically sign-extended.  */
	  for (; i < elem_bitsize; i += value_bit)
	    *vp++ = INTVAL (el) < 0 ? -1 : 0;
	  break;

But because of this assert, the equivalent meaning for
CONST_DOUBLE has never been defined, and the current code
happens to zero-extend it:

	case CONST_DOUBLE:
	  if (GET_MODE (el) == VOIDmode)
	    {
	      /* If this triggers, someone should have generated a
		 CONST_INT instead.  */
	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);

	      for (i = 0; i < HOST_BITS_PER_WIDE_INT; i += value_bit)
		*vp++ = CONST_DOUBLE_LOW (el) >> i;
	      while (i < HOST_BITS_PER_WIDE_INT * 2 && i < elem_bitsize)
		{
		  *vp++
		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
		  i += value_bit;
		}
	      /* It shouldn't matter what's done here, so fill it with
		 zero.  */
	      for (; i < elem_bitsize; i += value_bit)
		*vp++ = 0;
	    }

So the upshot is that:

    immed_double_const (-1, -1, 4_hwi_mode)

sign-extends i1 (the second -1), creating (-1, -1, -1, -1).  But:

    immed_double_const (0, -1, 4_hwi_mode)

effectively (as the code falls out at the moment) zero-extends it,
creating (0, -1, 0, 0).  That kind of inconsistency seems wrong.

So what I was trying to say was that if we remove the assert
altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs,
we need to define what the "implicit" high-order HWIs of a
CONST_DOUBLE are, just like we already do for CONST_INT.
If we remove the assert altogether, it very much matters
what is done by that last "*vp" line.

If Mike or anyone is up to doing that, then great.  But if instead
it's just a case of handling zero correctly, moving rather than
removing the assert seems safer.

I'm obviously not explaining this well :-)

Richard

Re: remove wrong code in immed_double_const

[Richard Guenther]

On Tue, Mar 20, 2012 at 1:26 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Guenther <richard.guenther@gmail.com> writes:
>>> I've no objection to moving the assert down to after the GEN_INT.
>>> But it sounds like I'm on my own with the whole CONST_DOUBLE sign thing.
>>> (That is, if we remove the assert altogether, we effectively treat the
>>> number as sign-extended if it happens to fit in a CONST_INT, and
>>> zero-extended otherwise.
>>
>> Why do we treat it zero-extended otherwise?  Because we use
>> gen_int_mode for CONST_INTs, which sign-extends?
>
> Just to make sure we're not talking past each other, I meant
> moving the assert to:
>
>    /* If this integer fits in one word, return a CONST_INT.  */
> [A] if ((i1 == 0 && i0 >= 0) || (i1 == ~0 && i0 < 0))
>      return GEN_INT (i0);
>
> <---HERE--->
>
>    /* We use VOIDmode for integers.  */
>    value = rtx_alloc (CONST_DOUBLE);
>    PUT_MODE (value, VOIDmode);
>
>    CONST_DOUBLE_LOW (value) = i0;
>    CONST_DOUBLE_HIGH (value) = i1;
>
>    for (i = 2; i < (sizeof CONST_DOUBLE_FORMAT - 1); i++)
>      XWINT (value, i) = 0;
>
>    return lookup_const_double (value);
>
> [A] treats i0 and i1 as a sign-extended value.  So if we
> removed the assert (or moved it to the suggested place):
>
>    immed_double_const (-1, -1, 4_hwi_mode)
>
> would create -1 in 4_hwi_mode, represented as a CONST_INT.
> The three implicit high-order HWIs are -1.  That's fine,
> because CONST_INT has long been defined as sign-extending
> rather than zero-extending.
>
> But if we fail the [A] test, we go on to create a CONST_DOUBLE.
> The problem is that AIUI we have never defined what happens for
> CONST_DOUBLE if the mode is wider than 2 HWIs.  Again AIUI,
> that's why the assert is there.
>
> This matters because of things like the handling in simplify_immed_subreg
> (which, e.g., we use to generate CONST_DOUBLE pool constants, split
> constant moves in lower-subreg.c, etc.).  CONST_INT is already
> well-defined to be a sign-extended constant, and we handle it correctly:
>
>      switch (GET_CODE (el))
>        {
>        case CONST_INT:
>          for (i = 0;
>               i < HOST_BITS_PER_WIDE_INT && i < elem_bitsize;
>               i += value_bit)
>            *vp++ = INTVAL (el) >> i;
>          /* CONST_INTs are always logically sign-extended.  */
>          for (; i < elem_bitsize; i += value_bit)
>            *vp++ = INTVAL (el) < 0 ? -1 : 0;
>          break;
>
> But because of this assert, the equivalent meaning for
> CONST_DOUBLE has never been defined, and the current code
> happens to zero-extend it:
>
>        case CONST_DOUBLE:
>          if (GET_MODE (el) == VOIDmode)
>            {
>              /* If this triggers, someone should have generated a
>                 CONST_INT instead.  */
>              gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
>
>              for (i = 0; i < HOST_BITS_PER_WIDE_INT; i += value_bit)
>                *vp++ = CONST_DOUBLE_LOW (el) >> i;
>              while (i < HOST_BITS_PER_WIDE_INT * 2 && i < elem_bitsize)
>                {
>                  *vp++
>                    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
>                  i += value_bit;
>                }
>              /* It shouldn't matter what's done here, so fill it with
>                 zero.  */
>              for (; i < elem_bitsize; i += value_bit)
>                *vp++ = 0;
>            }
>
> So the upshot is that:
>
>    immed_double_const (-1, -1, 4_hwi_mode)
>
> sign-extends i1 (the second -1), creating (-1, -1, -1, -1).  But:
>
>    immed_double_const (0, -1, 4_hwi_mode)
>
> effectively (as the code falls out at the moment) zero-extends it,
> creating (0, -1, 0, 0).  That kind of inconsistency seems wrong.
>
> So what I was trying to say was that if we remove the assert
> altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs,
> we need to define what the "implicit" high-order HWIs of a
> CONST_DOUBLE are, just like we already do for CONST_INT.
> If we remove the assert altogether, it very much matters
> what is done by that last "*vp" line.
>
> If Mike or anyone is up to doing that, then great.  But if instead
> it's just a case of handling zero correctly, moving rather than
> removing the assert seems safer.
>
> I'm obviously not explaining this well :-)

Ok, I see what you mean.  Yes, moving the assert past the GEN_INT
case (though that is specifically meant to deal with the VOIDmode case
I think?) is ok.

Thanks,
Richard.

> Richard

Re: remove wrong code in immed_double_const

[Michael Matz]

Hi,

On Tue, 20 Mar 2012, Richard Sandiford wrote:

> If Mike or anyone is up to doing that, then great.  But if instead it's 
> just a case of handling zero correctly, moving rather than removing the 
> assert seems safer.
> 
> I'm obviously not explaining this well :-)

Actually you did.  I've tried yesterday to come up with a text that would 
do the same (because I agree with you that deleting the assert changes 
the spec of the function, simply because the assert _is_ part of the 
spec of the function), and my attempt was _much_ worse than yours, so I 
didn't send it :)


Ciao,
Michael.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 20, 2012, at 5:26 AM, Richard Sandiford wrote:
> So what I was trying to say was that if we remove the assert
> altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs,
> we need to define what the "implicit" high-order HWIs of a
> CONST_DOUBLE are, just like we already do for CONST_INT.

The problem with your position is you are wrong.  Let me give you an concrete example:

typedef __attribute__((mode(OI))) signed int256_t;

int256_t foo() {
  return (int256_t)0x7000000000000000 << 10;
}

This fails your assert you want to move down.  This is _wrong_, wrong, wrong wrong.  This is getting tiring.

This is perfectly well defined, with no change in any definition.  Would you like to try again?

> If we remove the assert altogether, it very much matters what is done by that last "*vp" line.

> If Mike or anyone is up to doing that, then great.

I categorically object to moving it down, it is wrong, for all the same reasons the original is wrong.

> But if instead it's just a case of handling zero correctly, moving rather than
> removing the assert seems safer.

Sure, that fixes 1 testcase, and is monotonically better.  The problem, it fixes less than 1% of the values for which we assert currently which are wrong.  What is the point of bug pushing?  Bug pushing is wrong.

> I'm obviously not explaining this well :-)

No, you are explaining your position well, it is just that your position is wrong.  Your position is you want to bug push, that position is wrong.  You can't win with me by having that position.  The only solution is to change your position.  You've done it once now, and you are moving in the right direction.  You are now willing to remove the assert for 0, which you didn't want to do before.  Your next stop, will be to remove the assert for  (int256_t)0x7000000000000000 << 10.  I will keep pushing until you relent on that point.  Once you do,  your position will have changed twice, mine, unchanged.  I will not relent after that step, merely, I will select the next value and make you agree to it, then the next, and the next and the next, until I succeed in moving you to my original position.

Now, you might be thinking that this check will protect wrong code generation in the rest of the compiler, and that may be somewhat true.  A counter point would be:

  (int256_t)1 << 254

which would have been my next interesting value to get you to agree with me on, but, the assert you point at, doesn't prevent great badness from happening with this case.  Other parts of the compiler just suck, because people didn't care enough.  This I can change, but will take time.  In my first patch, it will not be complete nor perfect.  If you want to reject all the patches until every single last test case works, well, that isn't usually what we do.  We usually approve each one as long as it is monotonically better, that's the standard by which to judge patches.

Is your position, the rest of the compiler isn't perfect, so, no progress can be made in fixing it until the rest of it is perfect?  If so, this is a very bad position to take.  I have time now to fix and submit work to make things better.  After I have things working perfectly, I may have 0 time with which to do this.  I'll give you a concrete example which exactly parallels this work.  There was a time when gcc didn't work on 64-bit targets.  I did the first 64-bit port. It was done as a series of patches, one at a time, that pushed the compiler kicking and screaming in the right direction.  That work is the basis for all the 64-bit ports today; they are now less rare than they were when I first started.  gcc is reasonably good at supporting 64-bit ports.  What is different today, absolutely nothing, just N.  Instead of 64, it is 256, life goes on.  All I can say, if you have this mistaken notion that work to make OI work should be blocked because OI doesn't work, is you are wrong.  So, my question is, do you have this position?

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 20, 2012, at 6:55 AM, Michael Matz wrote:
> Actually you did.  I've tried yesterday to come up with a text that would 
> do the same (because I agree with you that deleting the assert changes 
> the spec of the function,

The spec of the function is the text above the definition of the function, coupled with the information in the .texi file, would you agree?  If so, could you please quote the text of the spec which would be violated by removing the assert?  Could you please give a specific value with which we could talk about that shows a violation of the spec.

My position is simple, the spec is what is above the definition and the .texi files, and the stuff inside the definition are interesting implementation details of that spec, which _never_ modify the spec.  My position is that 0 is a value which the spec defines, and for which we assert.  Please quote the line from the spec that defines what we do in that case.  I've never seen anyone quote such a line.  To support your position, I will insist on a direct quote from the  spec.

> simply because the assert _is_ part of the spec of the function), and my attempt was _much_ worse than yours, so I didn't send it :)

If you consider Eiffel, the pre and post condition on a function are indeed part of the spec of the function.  But, when they are wrong and need to be fixed, you can't argue that since the spec says if I is 42, abort, then trivially, we can't fix the spec because the spec says that if I is 42, we abort.  To back the position that spec must not be changed, you need to explain at least one thing for which the wrong thing will happen if the spec did change.  If you want to go down that path, you will need to furnish one example where badness happens with 0, not 2, not 3, but 0.  If you can't do that, you loose.  If you can, love to hear it.  Now, if you cite a buggy piece of software that does the wrong thing as support, I won't be swayed, I will concede the fact gcc has bugs and those bugs should be fixed, it always has, and always will.  See my other post for examples of existing bugs in gcc that are not protected by the assert.  I am sympathetic to preferring asserts over wrong code gen, so, I'd be willing to fix all the buggy routines or make them assert, before we loose the assert.  In that case, I'd really prefer a list of concrete places to fix.  An unbonded idea that the entire rest of the compiler needs fixing is, well, doesn't bode well for incremental forward progress.  Given just how buggy it is, personally, I don't see the problem in just declaring that OI is completely buggy, and move on.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 20, 2012, at 5:26 AM, Richard Sandiford wrote:
> So what I was trying to say was that if we remove the assert
> altogether, and allow CONST_DOUBLEs to be wider than 2 HWIs,
> we need to define what the "implicit" high-order HWIs of a
> CONST_DOUBLE are, just like we already do for CONST_INT.

Now, since you expressed a preference for sign extending, and a worry that there might be new bugs exposed in the handling of CONST_DOUBLEs in the face of my change, I went through all the code again and tried my best to fix every other bug in the compiler at all related to this area that I could find ; that patch is below.  In this one, I updated the spec for CONST_DOUBLE to be sign extending.

Curious, plus_constant is just terribly broken in this are, now fixed.  mode_signbit_p is speced in English, so, I didn't want to misread or misunderstand it and swizzle it, so I left it alone for now.   Someone will have to describe what it does and I can try my hand at fixing it, if broken, I suspect it is.  As for simplify_const_unary_operation, I don't know what they were thinking, return 0 seems safer to me.

If there is any other code that I missed that people know about, I'd be happy to fix it, just let me know what code.  I did a pass on all the ports as well, and they seem reasonably clean about it.  The biggest problem is OImode -1, would come out as hex digits, and all the upper 0xfffff digits implied by sign extension would be missing.  A port that cared about OImode, trivially, would fix their output routine.  The debugging code has similar problems.

Is this closer to something you think is in the right direction?  If so, let figure out the right solution for mode_signbit_p and proceed from there.


diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
index de45a22..0c6dc45 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode @var{m} or an
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+the value is a signed value, meaning the top bit of
+@code{CONST_DOUBLE_HIGH} is a sign bit.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 78ddfc3..c0b24e4 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
diff --git a/gcc/explow.c b/gcc/explow.c
index 2fae1a1..6284d61 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -96,6 +96,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
+	/* Punt for now.  */
+	goto overflow;
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -103,10 +106,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? ~(HOST_WIDE_INT)0 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
+	if (GET_MODE_BITSIZE (mode) > 2*HOST_WIDE_INT)
+	  /* Punt for now.  */
+	  goto overflow;
+
 	add_double (l1, h1, l2, h2, &lv, &hv);
 
 	return immed_double_const (lv, hv, VOIDmode);
@@ -141,6 +148,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
+      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
+	/* Punt for now.  */
+	goto overflow;
       /* The interesting case is adding the integer to a sum.
 	 Look for constant term in the sum and combine
 	 with C.  For an integer constant term, we make a combined
@@ -185,6 +195,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
     }
 
+ overflow:
   if (c != 0)
     x = gen_rtx_PLUS (mode, x, GEN_INT (c));
 
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index ce4eab4..37e46b1 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -101,6 +101,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
 
   if (width < HOST_BITS_PER_WIDE_INT)
     val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
+  /* FIXME: audit for TImode and wider correctness.  */
   return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));
 }
 

@@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
 	    return 0;
 	}
       else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
+	return 0;
       else
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
@@ -4987,6 +4988,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -4999,13 +5001,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
+	      unsigned char extend = 0;
 	      long tmp[max_bitsize / 32];
 	      int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
 
@@ -5030,10 +5034,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		  *vp++ = tmp[ibase / 32] >> i % 32;
 		}
 
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  break;
 

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texi
> index de45a22..0c6dc45 100644
> --- a/gcc/doc/rtl.texi
> +++ b/gcc/doc/rtl.texi
> @@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode @var{m} or an
>  integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
>  bits but small enough to fit within twice that number of bits (GCC
>  does not provide a mechanism to represent even larger constants).  In
> -the latter case, @var{m} will be @code{VOIDmode}.
> +the latter case, @var{m} will be @code{VOIDmode}.  For integral values
> +the value is a signed value, meaning the top bit of
> +@code{CONST_DOUBLE_HIGH} is a sign bit.
>  
>  @findex CONST_DOUBLE_LOW
>  If @var{m} is @code{VOIDmode}, the bits of the value are stored in

Sounds good.

> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 78ddfc3..c0b24e4 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
>  
>       1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>  	gen_int_mode.
> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
> -	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
> -	from copies of the sign bit, and sign of i0 and i1 are the same),  then
> -	we return a CONST_INT for i0.
> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
> +        (i.e., i1 consists only from copies of the sign bit, and sign
> +	of i0 and i1 are the same), then we return a CONST_INT for i0.
>       3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
>    if (mode != VOIDmode)
>      {

This too.

> diff --git a/gcc/explow.c b/gcc/explow.c
> index 2fae1a1..6284d61 100644
> --- a/gcc/explow.c
> +++ b/gcc/explow.c
> @@ -96,6 +96,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)

For this I think we should make plus_constant a wrapper:

/* Return an rtx for the sum of X and the integer C.  */

rtx
plus_constant (rtx x, HOST_WIDE_INT c)
{
  return plus_constant_mode (GET_MODE (x), x, c);
}

/* Return an rtx for the sum of X and the integer C, given that X
   has mode MODE.  */

rtx
plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
{
  ...innards of current plus_constant, without the "mode = "...
}

Reason being...

>    switch (code)
>      {
>      case CONST_INT:
> +      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> +	/* Punt for now.  */
> +	goto overflow;
>        return GEN_INT (INTVAL (x) + c);
>  
>      case CONST_DOUBLE:

...this won't work as things stand, since CONST_INT always has VOIDmode.
(I'm on a slow mission to fix that.)

I agree that this is a pre-existing bug.  Callers that want to be
CONST_INT-safe should use the new plus_constant_mode instead of
plus_constant.  Once they do, we should assert here that mode isn't
VOIDmode.  But since it's an existing bug that also affects 2-HWI
constants, I agree that replacing calls to plus_constant with calls
to plus_constant_mode is a separate fix.

I don't think it's a good idea to punt to a PLUS though.
(plus (const_int X) (const_int Y)) isn't canonical rtl,
and could cause other problems.

Suggest instead we reuse the CONST_DOUBLE code for CONST_INT,
with l1 set to INTVAL and h1 set to the sign extension.

> @@ -103,10 +106,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>  	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
>  	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
>  	unsigned HOST_WIDE_INT l2 = c;
> -	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
> +	HOST_WIDE_INT h2 = c < 0 ? ~(HOST_WIDE_INT)0 : 0;
>  	unsigned HOST_WIDE_INT lv;
>  	HOST_WIDE_INT hv;
>  
> +	if (GET_MODE_BITSIZE (mode) > 2*HOST_WIDE_INT)
> +	  /* Punt for now.  */
> +	  goto overflow;
> +
>  	add_double (l1, h1, l2, h2, &lv, &hv);

Nicely, add_double returns true on overflow, so I think
we should replace the punt with:

   if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
     gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);

(Seems better to explicitly specify the sign, even though
add_double would be equivalent.)

> @@ -141,6 +148,9 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>        break;
>  
>      case PLUS:
> +      if (GET_MODE_BITSIZE (mode) > HOST_WIDE_INT)
> +	/* Punt for now.  */
> +	goto overflow;
>        /* The interesting case is adding the integer to a sum.
>  	 Look for constant term in the sum and combine
>  	 with C.  For an integer constant term, we make a combined

For this I think we should change the recursive CONSTANT_P call
to use plus_constant_mode (with the mode of the PLUS) instead of
plus_constant.  It will then be correct for CONST_INT, and we can
remove the special CONST_INT case.

> diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
> index ce4eab4..37e46b1 100644
> --- a/gcc/simplify-rtx.c
> +++ b/gcc/simplify-rtx.c
> @@ -101,6 +101,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
>  
>    if (width < HOST_BITS_PER_WIDE_INT)
>      val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
> +  /* FIXME: audit for TImode and wider correctness.  */
>    return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));

We've already returned false in that case though.  I'm happy for this
function to be left as-is, but we could instead add a comment like:

    /* FIXME: We don't yet have a representation for wider modes.  */

above the:

    return false;

> @@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>  	    return 0;
>  	}
>        else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
> -	;
> +	return 0;
>        else
>  	hv = 0, lv &= GET_MODE_MASK (op_mode);

Should keep the current behaviour for == HOST_BITS_PER_WIDE_INT * 2.

I'd slightly prefer an assert rather than a "return false", but I won't
argue if another maintainer agrees with the return.

> @@ -4987,6 +4988,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  	case CONST_DOUBLE:
>  	  if (GET_MODE (el) == VOIDmode)
>  	    {
> +	      unsigned char extend = 0;
>  	      /* If this triggers, someone should have generated a
>  		 CONST_INT instead.  */
>  	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
> @@ -4999,13 +5001,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
>  		  i += value_bit;
>  		}
> -	      /* It shouldn't matter what's done here, so fill it with
> -		 zero.  */
> +
> +	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
> +		extend = -1;
>  	      for (; i < elem_bitsize; i += value_bit)
> -		*vp++ = 0;
> +		*vp++ = extend;
>  	    }

Looks good.

>  	  else
>  	    {
> +	      unsigned char extend = 0;
>  	      long tmp[max_bitsize / 32];
>  	      int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
>  
> @@ -5030,10 +5034,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
>  		  *vp++ = tmp[ibase / 32] >> i % 32;
>  		}
>  
> -	      /* It shouldn't matter what's done here, so fill it with
> -		 zero.  */
> +	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
> +		extend = -1;
>  	      for (; i < elem_bitsize; i += value_bit)
> -		*vp++ = 0;
> +		*vp++ = extend;
>  	    }

This is changing the real case, where sign extension doesn't make
much sense.

I think the expand_mult CONST_DOUBLE code needs changing too:

	  else if (CONST_DOUBLE_LOW (op1) == 0
		   && EXACT_POWER_OF_2_OR_ZERO_P (CONST_DOUBLE_HIGH (op1)))
	    {
	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
			  + HOST_BITS_PER_WIDE_INT;
	      return expand_shift (LSHIFT_EXPR, mode, op0,
				   shift, target, unsignedp);
	    }

This isn't correct if the mode is wider than 2 HWIs and
CONST_DOUBLE_HIGH has the high bit set.

Same for:

      /* Likewise for multipliers wider than a word.  */
      if (GET_CODE (trueop1) == CONST_DOUBLE
	  && (GET_MODE (trueop1) == VOIDmode
	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
	  && GET_MODE (op0) == mode
	  && CONST_DOUBLE_LOW (trueop1) == 0
	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
	return simplify_gen_binary (ASHIFT, mode, op0,
				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));

in the MULT case of simplify_binary_operation_1.

simplify_const_unary_operation needs to check for overflow
when handling 2-HWI arithmetic, since we can no longer assume
that the result is <= 2 HWIs in size.  E.g.:

	case NEG:
	  neg_double (l1, h1, &lv, &hv);
	  break;

should probably be:

	case NEG:
	  if (neg_double (l1, h1, &lv, &hv))
            gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);
	  break;

and similarly for other cases.

Richard

Re: remove wrong code in immed_double_const

[Michael Matz]

Hi,

On Tue, 20 Mar 2012, Mike Stump wrote:

> > Actually you did.  I've tried yesterday to come up with a text that 
> > would do the same (because I agree with you that deleting the assert 
> > changes the spec of the function,
> 
> The spec of the function is the text above the definition of the 
> function, coupled with the information in the .texi file, would you 
> agree?

Actually, I wouldn't.  The real spec includes many pieces of information, 
the comments (that can be incomplete or become out of date), the .texi 
docu (which can be even more incomplete and out of date), the code (which 
can conflict with the comments and still be the correct variant) and the 
current usage (which can conflict with everything of the above).  asserts 
are IMO even a nice way of documenting parts of the spec.

> If so, could you please quote the text of the spec which would 
> be violated by removing the assert?  Could you please give a specific 
> value with which we could talk about that shows a violation of the spec.

Richard did so.  If the high bit of i1 is set then currently you will get 
a negative number produced no matter the absolute value of it.  That's IMO 
part of the spec, high bit set --> negative number.  negative as defined 
by the various routines dealing with CONST_INT or CONST_DOUBLE interpreted 
in the modes allowed for creating them.

If you were to allow modes of larger size than what could be completely 
specified with the two HWI arguments i0 and i1, then it suddenly depends 
on the absolute value if you get a negative or positive number.  For small 
negative numbers (those for which i1 is ~0 and i0 < 0) you'll still get a 
negative CONST_INT.  For large negative numbers you'll get a CONST_DOUBLE, 
that when interpreted in the large requested mode (which is the only thing 
that makes sense) is positive.  It doesn't matter that it's still 
negative when interpreted in a smaller mode.

Hence all values where i1 is between (HWI)1 << (hwibits-1)) and 
((HWI)~0)-1 are the values you're searching for, that show the problem.  
As you correctly note the routine will of course generate the exact same 
CONST_DOUBLE object no matter the mode given, but they have to be 
interpreted together with the given mode.

This positive/negative inconsistency doesn't make sense to allow, and the 
assert ensures that it isn't allowed.

Now, this inconsistency can also be avoided via different means.  By 
extending the block comment in front of the function for instance, but 
then the assert would make even more sense.  So Richards proposal to move 
the assert is better: The problem occurs only with large positive or 
negative values (i1 not 0 or ~0), so the mode-size check can be moved 
after the GEN_INT call.

This would have the seemingly strange effect of disallowing too large 
modes only for large values, but that's simply a side-effect of 
CONST_DOUBLE and the whole associated machinery not being able to 
consistently deal with constants wider than 2*HWI_BITS.

> My position is simple, the spec is what is above the definition and the 
> .texi files, and the stuff inside the definition are interesting 
> implementation details of that spec, which _never_ modify the spec.

As an abstract goal that's good.  But reality is that this isn't the case.  
GCC is quite excellently commented, but it doesn't fit the ideal.  Using 
the fact that it isn't ideal to claim that the spec doesn't say anything 
about large modes (when the assert clearly disallows them) is absurd.

> My position is that 0 is a value which the spec defines, and for which 
> we assert.  Please quote the line from the spec that defines what we do 
> in that case.  I've never seen anyone quote such a line.  To support 
> your position, I will insist on a direct quote from the spec.

This line disallows the value 0 with large modes:

      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);

I insist on it being part of the spec.  Moving the assert changes the spec 
to allow 0 (and generally small positive and negative numbers) to also be 
generated for larger modes.  If you so want to change the spec nobody 
would be opposed.

> if I is 42, we abort.  To back the position that spec must not be 
> changed, you need to explain at least one thing for which the wrong 
> thing will happen if the spec did change.  If you want to go down that 
> path, you will need to furnish one example where badness happens with 0, 
> not 2, not 3, but 0.

Huh.  Removing the assert wouldn't only allow 0, but also other values.  
I don't understand your argumentation: "because for 0 nothing bad happens, 
that proves that nothing bad happens for any other values which we would 
also allow, hence I can remove the assert"?  It of course doesn't prove 
anything at all.  In any case, above I have given the values that will be 
problematic (and they don't include 0), and a way of changing the spec to 
disallow only them, instead of all values.  Actually Richard S. did so, I 
just repeated him.


Ciao,
Michael.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 21, 2012, at 6:47 AM, Michael Matz wrote:
> Actually, I wouldn't.

Ok, thanks for explaining.  In light of that, I'd just say, I want to change the spec, the details don't change any for me, only the terminology I might use.  The problem is the spec is causing aborts on valid code, and hence, either, the code must be duplicated and fixed, or the code has to be fixed.  I don't see any value in duplicating the code, so, I am left with fixing the spec so that valid programs produce valid code.

> If the high bit of i1 is set then currently you will get 
> a negative number produced no matter the absolute value of it.

Ok, in the new patch, I'm pushing to change the spec so that the value is sign extended and fixing all the code that doesn't conform to that spec.  Richard seems to be agreeable with the basic idea, though, we are now sorting out all the little details to make that happen.  If there is any down-side or details we missed or got wrong, please chime in.

> For large negative numbers you'll get a CONST_DOUBLE, 
> that when interpreted in the large requested mode (which is the only thing 
> that makes sense) is positive.

In the new patch, we use sign extension, and when the high bit is set, the value is interpreted as a negative number is a larger mode.  I'll test signed and unsigned constants coming in from above to ensure the right thing happens.  Above the signededness is tracked via the type.  In the rtl constant, it isn't, so that code will need an assert to prevent large unsigned values from taking this code path.

> Hence all values where i1 is between (HWI)1 << (hwibits-1)) and 
> ((HWI)~0)-1 are the values you're searching for, that show the problem.

Presently, I am fixing _all_ problems shown with those values.  If you know of any that we don't address, love to hear about them.

> This positive/negative inconsistency doesn't make sense to allow, and the 
> assert ensures that it isn't allowed.

Don't need the assert when there is no inconsistency, I believe that resolving any inconsistencies should remove the need for the assert.

> This would have the seemingly strange effect of disallowing too large 
> modes only for large values, but that's simply a side-effect of 
> CONST_DOUBLE and the whole associated machinery not being able to 
> consistently deal with constants wider than 2*HWI_BITS.

I'll move that assert up to the code that has the type information for the constant.

>> if I is 42, we abort.  To back the position that spec must not be 
>> changed, you need to explain at least one thing for which the wrong 
>> thing will happen if the spec did change.  If you want to go down that 
>> path, you will need to furnish one example where badness happens with 0, 
>> not 2, not 3, but 0.
> 
> Huh.  Removing the assert wouldn't only allow 0, but also other values.  
> I don't understand your argumentation: "because for 0 nothing bad happens, 
> that proves that nothing bad happens for any other values which we would 
> also allow, hence I can remove the assert"?

Right, it merely proves that the assert is wrong and needs fixing.  Once you accept that, then we progress on exactly what it should be.  This is now all mostly moot, given that I'm fine with changing the spec as Richard suggested to be a sign-extended constant.  Once we have that nice are concrete definition, the any code conflicts with that, is buggy, and we just fix.  Seems like a nice way forward to me.

> It of course doesn't prove anything at all.

:-)  Only the point I wanted to make; that 0 is safe.  As such, it proves that the spec, as you might call it, is wrong.  Once that spec is proven wrong, trivially, fixing it, isn't unreasonable.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 21, 2012, at 6:17 AM, Richard Sandiford wrote:
> Sounds good.

Cool, a path forward.

> For this I think we should make plus_constant a wrapper:

Ah, yes, much better, thanks.  I'd expanded the comments on plus_constant_mode so that one might have a better idea why the code is that way and put in a TODO, so that people have an idea of what direction the code wants to move.

> I don't think it's a good idea to punt to a PLUS though.

Fixed, thanks for the code.

> Nicely, add_double returns true on overflow, so I think
> we should replace the punt with:

Ah, yes, nice, fixed.

> For this I think we should change the recursive CONSTANT_P call
> to use plus_constant_mode

Done.

> and we can remove the special CONST_INT case.

Ok, ah, yes, because the recursive call will already combine them, done.

>>   if (width < HOST_BITS_PER_WIDE_INT)
>>     val &= ((unsigned HOST_WIDE_INT) 1 << width) - 1;
>> +  /* FIXME: audit for TImode and wider correctness.  */
>>   return val == ((unsigned HOST_WIDE_INT) 1 << (width - 1));
> 
> We've already returned false in that case though.

Ah, I saw it, but missed it anyway, thanks for the pointer, fixed.

> I'm happy for this function to be left as-is, but we could instead add a comment like:
> 
>    /* FIXME: We don't yet have a representation for wider modes.  */

Done.

>> @@ -1227,7 +1228,7 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>> 	    return 0;
>> 	}
>>       else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
>> -	;
>> +	return 0;
>>       else
>> 	hv = 0, lv &= GET_MODE_MASK (op_mode);
> 
> Should keep the current behaviour for == HOST_BITS_PER_WIDE_INT * 2.


> I'd slightly prefer an assert rather than a "return false", but I won't
> argue if another maintainer agrees with the return.

Ah, yes, I agree an assert would be better, as really, no one should ask for an unsigned conversion to floating point on a value that is negative, more likely it is just a spot we missed adding an assert to earlier and probably wants a larger constant that can represent a large unsigned number.  Fixed.

> This is changing the real case, where sign extension doesn't make
> much sense.

I'm not sure I followed.  Do you want me to remove the change for the second case, leave it as it, or something else?  I've left it as I had modified it.

> I think the expand_mult CONST_DOUBLE code needs changing

Agreed.  Fixed.  I preserve the code for smaller modes, and for small enough shift amounts. 1<<67 for example, or any mode, is still handled.  For large enough things, we just don't return the shift.

> Same for:

Fixed, in same way as previous case.

> simplify_const_unary_operation needs to check for overflow
> when handling 2-HWI arithmetic, since we can no longer assume
> that the result is <= 2 HWIs in size.  E.g.:
> 
> 	case NEG:
> 	  neg_double (l1, h1, &lv, &hv);
> 	  break;
> 
> should probably be:
> 
> 	case NEG:
> 	  if (neg_double (l1, h1, &lv, &hv))
>            gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);
> 	  break;

Are you talking about the block of code inside:

  /* We can do some operations on integer CONST_DOUBLEs.  Also allow                                                                              
     for a DImode operation on a CONST_INT.  */
  else if (GET_MODE (op) == VOIDmode
           && width <= HOST_BITS_PER_WIDE_INT * 2

?  If so, we already know that the width is <= HOST_BITS_PER_WIDE_INT * 2.  :-)

Thanks for spotting the bits I missed.


Current patch:

diff --git a/gcc/doc/rtl.texi b/gcc/doc/rtl.texiindex de45a22..0c6dc45 100644
--- a/gcc/doc/rtl.texi
+++ b/gcc/doc/rtl.texi
@@ -1530,7 +1530,9 @@ Represents either a floating-point constant of mode @var{m} or an
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+the value is a signed value, meaning the top bit of
+@code{CONST_DOUBLE_HIGH} is a sign bit.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 78ddfc3..c0b24e4 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
diff --git a/gcc/explow.c b/gcc/explow.c
index 2fae1a1..c94ad25 100644
--- a/gcc/explow.c
+++ b/gcc/explow.c
@@ -73,14 +73,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enum machine_mode mode)
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when x can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is smaller than the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -90,12 +96,26 @@ plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT-1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -103,11 +123,16 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  if (GET_MODE_BITSIZE (mode) > 2*HOST_BITS_PER_WIDE_INT)
+	    /* Sorry, we have no way to represent overflows this
+	       wide.  To fix, add constant support wider than
+	       CONST_DOUBLE.  */
+	    gcc_assert (0);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -121,8 +146,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	{
 	  tem
 	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+			       plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)),
+						   c));
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -141,31 +166,17 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* The interesting case is adding the integer to a sum.
-	 Look for constant term in the sum and combine
-	 with C.  For an integer constant term, we make a combined
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* The interesting case is adding the integer to a sum.  Look
+	 for constant term in the sum and combine with C.  For an
+	 integer constant term or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -175,7 +186,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -195,6 +206,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* Return an rtx for the sum of X and the integer C.  */
+
+rtx
+plus_constant (rtx x, HOST_WIDE_INT c)
+{
+  return plus_constant_mode (GET_MODE (x), x, c);
+}
 

 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
diff --git a/gcc/expmed.c b/gcc/expmed.c
index 3507dad..2361b7e 100644
--- a/gcc/expmed.c
+++ b/gcc/expmed.c
@@ -3122,9 +3122,11 @@ expand_mult (enum machine_mode mode, rtx op0, rtx op1, rtx target,
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   build_int_cst (NULL_TREE, shift),
-				   target, unsignedp);
+	      if (shift < 2*HOST_BITS_PER_WIDE_INT-1
+		  || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     build_int_cst (NULL_TREE, shift),
+				     target, unsignedp);
 	    }
 	}
 
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 66f2755..9d1a28e 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -1586,6 +1586,7 @@ extern int ceil_log2 (unsigned HOST_WIDE_INT);
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);
diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c
index ce4eab4..ff838a8 100644
--- a/gcc/simplify-rtx.c
+++ b/gcc/simplify-rtx.c
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, const_rtx x)
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1219,15 +1220,10 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
+      if (op_mode == VOIDmode
+	  || GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
+	/* We should never get a negative number.  */
+	gcc_assert (hv >= 0);
       else
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
@@ -2214,7 +2210,9 @@ simplify_binary_operation_1 (enum rtx_code code, enum machine_mode mode,
 	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
 	  && GET_MODE (op0) == mode
 	  && CONST_DOUBLE_LOW (trueop1) == 0
-	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2*HOST_BITS_PER_WIDE_INT-1
+	      || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -4987,6 +4985,7 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -4999,13 +4998,15 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
+	      unsigned char extend = 0;
 	      long tmp[max_bitsize / 32];
 	      int bitsize = GET_MODE_BITSIZE (GET_MODE (el));
 
@@ -5030,10 +5031,10 @@ simplify_immed_subreg (enum machine_mode outermode, rtx op,
 		  *vp++ = tmp[ibase / 32] >> i % 32;
 		}
 
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT-1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  break;
 

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
>> This is changing the real case, where sign extension doesn't make
>> much sense.
>
> I'm not sure I followed.  Do you want me to remove the change for the
> second case, leave it as it, or something else?  I've left it as I had
> modified it.

Sorry, meant we should leave the svn version as it is.  The new docs
(rightly) only mention sign-extension for integer modes, so the comment
about it not mattering for FP modes is still correct.  (In principle
I'd prefer to replace it with an assert, but that's a separate change.
Nothing we're doing here should change whether the FP path gets executed.)

>> simplify_const_unary_operation needs to check for overflow
>> when handling 2-HWI arithmetic, since we can no longer assume
>> that the result is <= 2 HWIs in size.  E.g.:
>> 
>> 	case NEG:
>> 	  neg_double (l1, h1, &lv, &hv);
>> 	  break;
>> 
>> should probably be:
>> 
>> 	case NEG:
>> 	  if (neg_double (l1, h1, &lv, &hv))
>>            gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_WIDE_INT);
>> 	  break;
>
> Are you talking about the block of code inside:
>
>   /* We can do some operations on integer CONST_DOUBLEs.  Also allow                                                                              
>      for a DImode operation on a CONST_INT.  */
>   else if (GET_MODE (op) == VOIDmode
>            && width <= HOST_BITS_PER_WIDE_INT * 2
>
> ?

Heh. it seems so :-)  Never mind that bit then.

> diff --git a/gcc/explow.c b/gcc/explow.c
> index 2fae1a1..c94ad25 100644
> --- a/gcc/explow.c
> +++ b/gcc/explow.c
> @@ -73,14 +73,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enum machine_mode mode)
>    return c;
>  }
>  
> -/* Return an rtx for the sum of X and the integer C.  */
> +/* Return an rtx for the sum of X and the integer C, given that X has
> +   mode MODE.  This routine should be used instead of plus_constant
> +   when they want to ensure that addition happens in a particular
> +   mode, which is necessary when x can be a VOIDmode CONST_INT or
> +   CONST_DOUBLE and the width of the constant is smaller than the
> +   width of the expression.  */

I think this should be s/is smaller than/is different from/.
We're in trouble whenever the width of the HWIs is different
from the width of the constant they represent.

> @@ -103,11 +123,16 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>  	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
>  	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
>  	unsigned HOST_WIDE_INT l2 = c;
> -	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
> +	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
>  	unsigned HOST_WIDE_INT lv;
>  	HOST_WIDE_INT hv;
>  
> -	add_double (l1, h1, l2, h2, &lv, &hv);
> +	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
> +	  if (GET_MODE_BITSIZE (mode) > 2*HOST_BITS_PER_WIDE_INT)
> +	    /* Sorry, we have no way to represent overflows this
> +	       wide.  To fix, add constant support wider than
> +	       CONST_DOUBLE.  */
> +	    gcc_assert (0);

Should be:

	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
	  /* Sorry, we have no way to represent overflows this
	     wide.  To fix, add constant support wider than
	     CONST_DOUBLE.  */
	  gcc_assert (GET_MODE_BITSIZE (mode) > 2 * HOST_BITS_PER_WIDE_INT)

(note spacing).

> @@ -121,8 +146,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
>  	{
>  	  tem
>  	    = force_const_mem (GET_MODE (x),
> -			       plus_constant (get_pool_constant (XEXP (x, 0)),
> -					      c));
> +			       plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)),
> +						   c));
>  	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
>  	    return tem;
>  	}

Nitlet, but line is wider than 80 chars.  Probably easiest fix is:

	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
	  tem = force_const_mem (GET_MODE (x), tem);

> diff --git a/gcc/expmed.c b/gcc/expmed.c
> index 3507dad..2361b7e 100644
> --- a/gcc/expmed.c
> +++ b/gcc/expmed.c
> @@ -3122,9 +3122,11 @@ expand_mult (enum machine_mode mode, rtx op0, rtx op1, rtx target,
>  	    {
>  	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
>  			  + HOST_BITS_PER_WIDE_INT;
> -	      return expand_shift (LSHIFT_EXPR, mode, op0,
> -				   build_int_cst (NULL_TREE, shift),
> -				   target, unsignedp);
> +	      if (shift < 2*HOST_BITS_PER_WIDE_INT-1
> +		  || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT)

Missing spaces around binary operators.

> @@ -1219,15 +1220,10 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>        else
>  	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
>  
> -      if (op_mode == VOIDmode)
> -	{
> -	  /* We don't know how to interpret negative-looking numbers in
> -	     this case, so don't try to fold those.  */
> -	  if (hv < 0)
> -	    return 0;
> -	}
> -      else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
> -	;
> +      if (op_mode == VOIDmode
> +	  || GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
> +	/* We should never get a negative number.  */
> +	gcc_assert (hv >= 0);
>        else
>  	hv = 0, lv &= GET_MODE_MASK (op_mode);
>  

Sorry, with this bit, I meant that the current svn code is correct
for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2.
In that case, hv < 0 can just mean that we have a uint128_t
(or whatever) whose high bit happens to be set.  I think it
should be something like:

      if (op_mode == VOIDmode
	  || GET_MODE_BITSIZE (op_mode) > HOST_BITS_PER_WIDE_INT * 2)
	/* We should never get a negative number.  */
	gcc_assert (hv >= 0);
      else if (GET_MODE_BITSIZE (op_mode) <= HOST_BITS_PER_WIDE_INT)
	hv = 0, lv &= GET_MODE_MASK (op_mode);

> @@ -2214,7 +2210,9 @@ simplify_binary_operation_1 (enum rtx_code code, enum machine_mode mode,
>  	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
>  	  && GET_MODE (op0) == mode
>  	  && CONST_DOUBLE_LOW (trueop1) == 0
> -	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
> +	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
> +	  && (val < 2*HOST_BITS_PER_WIDE_INT-1
> +	      || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT))
>  	return simplify_gen_binary (ASHIFT, mode, op0,
>  				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
>  

Missing spaces around binary operators.

OK with those changes as far as the RTL optimisations go.  I'm happy
with the rest too, but despite all this fuss, I can't approve the
immed_double_const change itself.  Sounds like Richard G would be
willing though.

Richard

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Should be:
>
> 	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
> 	  /* Sorry, we have no way to represent overflows this
> 	     wide.  To fix, add constant support wider than
> 	     CONST_DOUBLE.  */
> 	  gcc_assert (GET_MODE_BITSIZE (mode) > 2 * HOST_BITS_PER_WIDE_INT)

Er, I did of course mean:

	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)

:-)

Richard

Re: remove wrong code in immed_double_const

[Michael Matz]

Hi,

On Wed, 21 Mar 2012, Mike Stump wrote:

> > If the high bit of i1 is set then currently you will get a negative 
> > number produced no matter the absolute value of it.
> 
> Ok, in the new patch, I'm pushing to change the spec so that the value 
> is sign extended and fixing all the code that doesn't conform to that 
> spec.

That certainly is strictly better than any of the other possibilities, I 
just didn't get the impression from your second mail to this thread that 
you were even considering doing that.  Good I was wrong.  If I notice 
anything I'll answer directly to the patch.

> > This positive/negative inconsistency doesn't make sense to allow, and 
> > the assert ensures that it isn't allowed.
> 
> Don't need the assert when there is no inconsistency, I believe that 
> resolving any inconsistencies should remove the need for the assert.

Correct.

> :-)  Only the point I wanted to make; that 0 is safe.  As such, it 
> proves that the spec, as you might call it, is wrong.

I would call it too strict, not wrong.  Because there are (or were after 
your fixes get it) values where there was a problem.  Of course that's 
again just splitting hair about terminology :)


Ciao,
Michael.

Re: remove wrong code in immed_double_const

[Michael Matz]

Hi,

On Wed, 21 Mar 2012, Mike Stump wrote:

> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -531,10 +531,9 @@ immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
>  
>       1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
>  	gen_int_mode.
> -     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
> -	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
> -	from copies of the sign bit, and sign of i0 and i1 are the same),  then
> -	we return a CONST_INT for i0.
> +     2) If the value of the integer fits into HOST_WIDE_INT anyway
> +        (i.e., i1 consists only from copies of the sign bit, and sign
> +	of i0 and i1 are the same), then we return a CONST_INT for i0.

I see that you didn't remove the assert as part of this patch.  I'd like 
to see what you like to do to this routine once the rest goes in.  In 
particular I don't think just removing the assert will be enough, at the 
very least the block comment should be saying something about what the 
routine exactly does (or doesn't do) for modes where the two HWI arguments 
can't specify all bits.

My point is, for large modes i0 and i1 will only specify the low 2*HWIbits 
bits.  Something needs to document what the upper bits will be (be they 
implicit or explicit), otherwise people reading the comment will always 
wonder what exactly is supposed to happen.  I'm not 100% sure what it 
should say, though.  Probably the interpretation of the upper bits depends 
on the users of the so generated CONST_DOUBLEs.


Ciao,
Michael.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 22, 2012, at 6:15 AM, Michael Matz wrote:
> That certainly is strictly better than any of the other possibilities, I 
> just didn't get the impression from your second mail to this thread that 
> you were even considering doing that.  Good I was wrong.

All I wanted, was to remove the assert...  The review point was, no, not unless you fix the issues so we don't get wrong code-gen and could you make it sign extending as well?  So, sure, sounds reasonable to me.  I was going to do the work in the end, just didn't plan on doing it today.  Today, tomorrow, not worth quibbling over the exact date the work is done.  But, my final point is, the assert is wrong, and it has to go, and (almost) gone it is.  :-)  I'm happy.

> I would call it too strict, not wrong.

Do you have users?  Have you ever told them the compiler isn't wrong when it ICEs for perfectly valid code?  I've never done that before, and never plan to, no one has convinced me it is the right approach.  If you want me to not use the term wrong, you'd need to furnish a web site that somehow proves your point.  Wrong is what I use when that the compiler does is wrong.  It is that simple.  Failing to compile valid code, is wrong.

> Because there are (or were after 
> your fixes get it) values where there was a problem.  Of course that's 
> again just splitting hair about terminology :)

Yeah, I'm not into hair splitting on terminology.  I'm more into actual functionality of the compiler to the end user.

Re: remove wrong code in immed_double_const

[Mike Stump]

On Mar 22, 2012, at 7:12 AM, Michael Matz wrote:
> I see that you didn't remove the assert as part of this patch.

I'll include that in my next patch.

> I'd like  to see what you like to do to this routine once the rest goes in.  In 
> particular I don't think just removing the assert will be enough, at the 
> very least the block comment should be saying something about what the 
> routine exactly does (or doesn't do) for modes where the two HWI arguments 
> can't specify all bits.

I think the best approach is to refine the spec:

/* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair                                                                                             
   of ints: I0 is the low-order word and I1 is the high-order word.                                                                                               
   The value is a signed value, with the high bit of i1 being the sign                                                                                            
   bit.  Do not use this routine for non-integer modes; convert to                                                                                                
   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */

I think this then exactly matches CONST_DOUBLE semantics.

Re: remove wrong code in immed_double_const

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 4456 bytes --]

On Mar 22, 2012, at 3:16 AM, Richard Sandiford wrote:
> Sorry, meant we should leave the svn version as it is.

Ah, I see now, I agree, I removed that bit (extend in the floating point case) of my change from the patch.

> I think this should be s/is smaller than/is different from/.

Sounds good, fixed.

> Should be:
> 
> 	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
> 	  /* Sorry, we have no way to represent overflows this
> 	     wide.  To fix, add constant support wider than
> 	     CONST_DOUBLE.  */
> 	  gcc_assert (GET_MODE_BITSIZE (mode) > 2 * HOST_BITS_PER_WIDE_INT)

As you noted, you do mean:

	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
          /* Sorry, we have no way to represent overflows this wide.                                                                                              
             To fix, add constant support wider than CONST_DOUBLE.  */
          gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)

Fixed.

> Nitlet, but line is wider than 80 chars.  Probably easiest fix is:
> 
> 	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
> 	  tem = force_const_mem (GET_MODE (x), tem);

Fixed.

>> +	      if (shift < 2*HOST_BITS_PER_WIDE_INT-1
>> +		  || GET_MODE_BITSIZE (mode) <= 2*HOST_BITS_PER_WIDE_INT)
> 
> Missing spaces around binary operators.

Fixed all instances of 2*HOST and INT-1, there were many, not just this one.  Some pre-date my patch.

>> @@ -1219,15 +1220,10 @@ simplify_const_unary_operation (enum rtx_code code, enum machine_mode mode,
>>       else
>> 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
>> 
>> -      if (op_mode == VOIDmode)
>> -	{
>> -	  /* We don't know how to interpret negative-looking numbers in
>> -	     this case, so don't try to fold those.  */
>> -	  if (hv < 0)
>> -	    return 0;
>> -	}
>> -      else if (GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
>> -	;
>> +      if (op_mode == VOIDmode
>> +	  || GET_MODE_BITSIZE (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
>> +	/* We should never get a negative number.  */
>> +	gcc_assert (hv >= 0);
>>       else
>> 	hv = 0, lv &= GET_MODE_MASK (op_mode);
>> 
> 
> Sorry, with this bit, I meant that the current svn code is correct
> for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2.
> In that case, hv < 0 can just mean that we have a uint128_t
> (or whatever) whose high bit happens to be set.

Well, according to the spec, one cannot use CONST_DOUBLE to represent a uint128 value with the high bit set.  The C frontend type plays this game, but they can, because they track the type with the constant the the values of the constant are interpreted exclusively in  the context of the type.  Since we don't have the unsigned bit, we can't, so, either, they are speced to be values on their own, or values dependent upon some external notion.  By changing the spec to say sign extending, we mean if the high bit is set, the value is negative.  If this is the wrong value for a uint value, then that representation cannot be used.  The only solution is to use a different representation (CONST_QUAD, CONST_UDOUBLE or something else). 

I'm thought about that routine some more, and it is just totally broken.  I've changed the code to:

      /* We should never get a negative number.  */
      gcc_assert (hv >= 0);

      if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
        hv = 0, lv &= GET_MODE_MASK (op_mode);

negative numbers are bad, period, this badness, doesn't depend upon anything other than the value being negative.  The use of hv = 0, can only be done with PRECISIONs less than or equal to HOST_BITS_PER_WIDE_INT.  Wider than this, and some of hv would be wiped out, which would be wrong.

Thoughts?

> OK with those changes as far as the RTL optimisations go.

Not quite.  Need to resolve the point just above.  But, yea, we are getting very close now.  I'm switched over to a patch for trunk (from 4.6.0) and added the ChangeLog.  The switch to GET_MODE_PRECISION above was one change, for example.  The rest fit in nicely, the arg to expand_shift was updated slightly to match trunk.

> I can't approve the immed_double_const change itself.  Sounds like Richard G would be
> willing though.

I've bundled in the patch that started this all into this version.  I'll look forward to his review.


[-- Attachment #2: oi-trunk.diffs.txt --]
[-- Type: text/plain, Size: 12469 bytes --]

	* doc/rtl.texi (const_double): Document as sign-extending.
	* expmed.c (expand_mult): Ensure we don't use shift
	incorrectly.
	* emit-rtl.c (immed_double_int_const): Refine to state the
	value is signed.
	* simplify-rtx.c (mode_signbit_p): Add a fixme for wider than
	CONST_DOUBLE integers.
	(simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no
	negative values are converted.  Fix conversions bigger than
	HOST_BITS_PER_WIDE_INT.
	(simplify_binary_operation_1): Ensure we don't use shift
	incorrectly.
	(simplify_immed_subreg): Sign-extend CONST_DOUBLEs.
	* explow.c (plus_constant_mode): Add.
	(plus_constant): Implement with plus_constant_mode.
	* rtl.h (plus_constant_mode): Add.

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 185706)
+++ doc/rtl.texi	(working copy)
@@ -1510,7 +1510,9 @@ Represents either a floating-point const
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+the value is a signed value, meaning the top bit of
+@code{CONST_DOUBLE_HIGH} is a sign bit.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
Index: expmed.c
===================================================================
--- expmed.c	(revision 185706)
+++ expmed.c	(working copy)
@@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   shift, target, unsignedp);
+	      if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1
+		  || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     shift, target, unsignedp);
 	    }
 	}
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 185706)
+++ emit-rtl.c	(working copy)
@@ -517,7 +517,8 @@ immed_double_int_const (double_int i, en
 
 /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
    of ints: I0 is the low-order word and I1 is the high-order word.
-   Do not use this routine for non-integer modes; convert to
+   The value is a signed value, with the high bit of i1 being the sign
+   bit.  Do not use this routine for non-integer modes; convert to
    REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
@@ -531,10 +532,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
@@ -546,8 +546,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */
Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 185706)
+++ simplify-rtx.c	(working copy)
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, 
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1355,16 +1356,10 @@ simplify_const_unary_operation (enum rtx
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
-      else
+      /* We should never get a negative number.  */
+      gcc_assert (hv >= 0);
+
+      if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
       REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode);
@@ -1718,7 +1713,7 @@ simplify_const_unary_operation (enum rtx
   else if (GET_CODE (op) == CONST_DOUBLE
 	   && SCALAR_FLOAT_MODE_P (GET_MODE (op))
 	   && GET_MODE_CLASS (mode) == MODE_INT
-	   && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0)
+	   && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0)
     {
       /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX
 	 operators are intentionally left unspecified (to ease implementation
@@ -1783,7 +1778,7 @@ simplify_const_unary_operation (enum rtx
 	    return const0_rtx;
 
 	  /* Test against the unsigned upper bound.  */
-	  if (width == 2*HOST_BITS_PER_WIDE_INT)
+	  if (width == 2 * HOST_BITS_PER_WIDE_INT)
 	    {
 	      th = -1;
 	      tl = -1;
@@ -2380,7 +2375,9 @@ simplify_binary_operation_1 (enum rtx_co
 	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
 	  && GET_MODE (op0) == mode
 	  && CONST_DOUBLE_LOW (trueop1) == 0
-	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2 * HOST_BITS_PER_WIDE_INT - 1
+	      || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -5189,6 +5186,7 @@ simplify_immed_subreg (enum machine_mode
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -5201,10 +5199,11 @@ simplify_immed_subreg (enum machine_mode
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT - 1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
Index: explow.c
===================================================================
--- explow.c	(revision 185706)
+++ explow.c	(working copy)
@@ -74,14 +74,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enu
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when x can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is different from the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -91,12 +97,26 @@ plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -104,11 +124,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  /* Sorry, we have no way to represent overflows this wide.
+	     To fix, add constant support wider than CONST_DOUBLE.  */
+	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -120,10 +143,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
 	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
 	{
-	  tem
-	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
+	  tem = force_const_mem (GET_MODE (x), tem);
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -142,31 +163,17 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* The interesting case is adding the integer to a sum.
-	 Look for constant term in the sum and combine
-	 with C.  For an integer constant term, we make a combined
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* The interesting case is adding the integer to a sum.  Look
+	 for constant term in the sum and combine with C.  For an
+	 integer constant term or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -176,7 +183,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -196,6 +203,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* Return an rtx for the sum of X and the integer C.  */
+
+rtx
+plus_constant (rtx x, HOST_WIDE_INT c)
+{
+  return plus_constant_mode (GET_MODE (x), x, c);
+}
 \f
 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
Index: rtl.h
===================================================================
--- rtl.h	(revision 185706)
+++ rtl.h	(working copy)
@@ -1644,6 +1644,7 @@ extern int ceil_log2 (unsigned HOST_WIDE
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);

[-- Attachment #3: Type: text/plain, Size: 3 bytes --]

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
>> Sorry, with this bit, I meant that the current svn code is correct
>> for GET_MODE_BITSIZE (op_mode) == HOST_BITS_PER_WIDE_INT * 2.
>> In that case, hv < 0 can just mean that we have a uint128_t
>> (or whatever) whose high bit happens to be set.

(To be clear, I was using uint128_t as an example of a 2-HWI type,
assuming we're using 64-bit HWIs -- which I hope we are for targets
where this assert matters.)

> Well, according to the spec, one cannot use CONST_DOUBLE to represent
> a uint128 value with the high bit set.

We can!  And do now, even without your patch.  Because...

> The C frontend type plays this game, but they can, because they track
> the type with the constant the the values of the constant are
> interpreted exclusively in the context of the type.  Since we don't
> have the unsigned bit, we can't, so, either, they are speced to be
> values on their own, or values dependent upon some external notion.
> By changing the spec to say sign extending, we mean if the high bit is
> set, the value is negative.

...it doesn't mean that we interpret the value as a negative _rtx_.
As with all rtx calculations, things like signedness and saturation are
decided by the operation rather than the "type" ("type" == rtx mode).
For things like addition where signed vs. unsigned interpretation
doesn't matter, we have a single rtx op like PLUS.  For things like
multiplication where it does matter, we have separate signed and
unsigned variants.  There is nothing to distinguish a uint128_t
_register_ (i.e. TImode REG) that has the upper bit set from an
int128_t register that happens to be negative.  Instead the
interpretation is decided by the operation.  And the same principle
applies to constants.  There isn't, and doesn't need to be,
a separate CONST_DOUBLE representation for:

  - an unsigned 2-HWI value that has the upper bit set and
  - a signed 2-HWI value that is negative

The sign-extending thing is simply there to specify what happens when an
N>2 HWI value is represented as a 2-HWI rtx.  I.e. it's simply there to
say what the implicit N-2 HWIs are.  (That's why the definition only
matters now that we're allowing N>2 by removing the assert.)

In this context we're interpreting the value as unsigned because we have
an UNSIGNED_FLOAT operation.  So if the mode of the operand is exactly
2 HWIs in size, a negative high HWI simply indicates an unsigned value
that has the high bit set.

The same principle already applies to CONST_INT.  We have long defined
CONST_INT to be a sign-extending representation, in the sense that it
is allowed to represent 2-HWI modes in which the upper HWI happens
to be a sign extension of the lower HWI.  That doesn't mean the 2-HWI
constant itself is negative: it can just as easily be a high unsigned
value.  Whether it is signed, unsigned or neutral depends on the context
of the rtx operation.

All we're doing here is extending that principle to CONST_DOUBLE
and modes wider than 2 HWIs.

Sorry for the rather rambling explanation :-)  I still think the
version I suggested for this hunk is right though.

Richard

Re: remove wrong code in immed_double_const

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote:
> ...it doesn't mean that we interpret the value as a negative _rtx_.
> As with all rtx calculations, things like signedness and saturation are
> decided by the operation rather than the "type" ("type" == rtx mode).

Ah...  [ light goes on ]  Let me adjust the documentation to be exceptionally clear in this case.  Check out the new wording on const_int, const_double and on immed_double_const.  I fixed simplify_const_unary_operation to match your suggestion.

> Sorry for the rather rambling explanation :-)  I still think the
> version I suggested for this hunk is right though.

I agree.  I now see what I had wrong.  Thanks for your patience and explanations.  If you like the wording I used in the doc and on immed_double_const, I think we have now resolved all issues.  The previous version was bootstraped and regression tested on darwin, fortran, c, c++, objective-c++...  I'll do one more run with the update for simplify_const_unary_operation, as that can trip when before it would merely return 0.

Ok?


[-- Attachment #2: oi-trunk-1.diffs.txt --]
[-- Type: text/plain, Size: 13134 bytes --]

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 185706)
+++ doc/rtl.texi	(working copy)
@@ -1479,8 +1479,13 @@ This type of expression represents the i
 is customarily accessed with the macro @code{INTVAL} as in
 @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
 
-Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
-must be sign extended to full width (e.g., with @code{gen_int_mode}).
+Constants generated for modes with fewer bits than in
+@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
+@code{gen_int_mode}).  For constants for modes with more bits than in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit, however values are neither signed nor unsigned.
+All operations defined on such constants define the signededness.
+
 
 @findex const0_rtx
 @findex const1_rtx
@@ -1510,7 +1515,12 @@ Represents either a floating-point const
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+constants for modes with more bits than twice the number in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral
+values are neither signed nor unsigned.  All operations defined on
+such constants define the signededness.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
Index: expmed.c
===================================================================
--- expmed.c	(revision 185706)
+++ expmed.c	(working copy)
@@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   shift, target, unsignedp);
+	      if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1
+		  || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     shift, target, unsignedp);
 	    }
 	}
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 185706)
+++ emit-rtl.c	(working copy)
@@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en
 
 /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
    of ints: I0 is the low-order word and I1 is the high-order word.
-   Do not use this routine for non-integer modes; convert to
-   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
+   For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the
+   implied upper bits are copies of the high bit of i1.  The value
+   itself is neither signed nor unsigned.  Do not use this routine for
+   non-integer modes; convert to REAL_VALUE_TYPE and use
+   CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
 immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
@@ -531,10 +534,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
@@ -546,8 +548,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */
Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 185706)
+++ simplify-rtx.c	(working copy)
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, 
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1355,16 +1356,11 @@ simplify_const_unary_operation (enum rtx
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
-      else
+      if (op_mode == VOIDmode
+	  || GET_MODE_PRECISION (op_mode) > 2 * HOST_BITS_PER_WIDE_INT)
+	/* We should never get a negative number.  */
+	gcc_assert (hv >= 0);
+      else if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
       REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode);
@@ -1718,7 +1714,7 @@ simplify_const_unary_operation (enum rtx
   else if (GET_CODE (op) == CONST_DOUBLE
 	   && SCALAR_FLOAT_MODE_P (GET_MODE (op))
 	   && GET_MODE_CLASS (mode) == MODE_INT
-	   && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0)
+	   && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0)
     {
       /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX
 	 operators are intentionally left unspecified (to ease implementation
@@ -1783,7 +1779,7 @@ simplify_const_unary_operation (enum rtx
 	    return const0_rtx;
 
 	  /* Test against the unsigned upper bound.  */
-	  if (width == 2*HOST_BITS_PER_WIDE_INT)
+	  if (width == 2 * HOST_BITS_PER_WIDE_INT)
 	    {
 	      th = -1;
 	      tl = -1;
@@ -2380,7 +2376,9 @@ simplify_binary_operation_1 (enum rtx_co
 	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
 	  && GET_MODE (op0) == mode
 	  && CONST_DOUBLE_LOW (trueop1) == 0
-	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2 * HOST_BITS_PER_WIDE_INT - 1
+	      || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -5189,6 +5187,7 @@ simplify_immed_subreg (enum machine_mode
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -5201,10 +5200,11 @@ simplify_immed_subreg (enum machine_mode
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT - 1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
Index: explow.c
===================================================================
--- explow.c	(revision 185706)
+++ explow.c	(working copy)
@@ -74,14 +74,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enu
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when x can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is different from the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -91,12 +97,26 @@ plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -104,11 +124,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  /* Sorry, we have no way to represent overflows this wide.
+	     To fix, add constant support wider than CONST_DOUBLE.  */
+	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -120,10 +143,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
 	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
 	{
-	  tem
-	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
+	  tem = force_const_mem (GET_MODE (x), tem);
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -142,31 +163,17 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* The interesting case is adding the integer to a sum.
-	 Look for constant term in the sum and combine
-	 with C.  For an integer constant term, we make a combined
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* The interesting case is adding the integer to a sum.  Look
+	 for constant term in the sum and combine with C.  For an
+	 integer constant term or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -176,7 +183,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -196,6 +203,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* Return an rtx for the sum of X and the integer C.  */
+
+rtx
+plus_constant (rtx x, HOST_WIDE_INT c)
+{
+  return plus_constant_mode (GET_MODE (x), x, c);
+}
 \f
 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
Index: rtl.h
===================================================================
--- rtl.h	(revision 185706)
+++ rtl.h	(working copy)
@@ -1644,6 +1644,7 @@ extern int ceil_log2 (unsigned HOST_WIDE
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);

Re: remove wrong code in immed_double_const

[Richard Sandiford]

Mike Stump <mikestump@comcast.net> writes:
> On Mar 23, 2012, at 3:01 AM, Richard Sandiford wrote:
>> ...it doesn't mean that we interpret the value as a negative _rtx_.
>> As with all rtx calculations, things like signedness and saturation are
>> decided by the operation rather than the "type" ("type" == rtx mode).
>
> Ah...  [ light goes on ]  Let me adjust the documentation to be exceptionally clear in this case.  Check out the new wording on const_int, const_double and on immed_double_const.  I fixed simplify_const_unary_operation to match your suggestion.
>
>> Sorry for the rather rambling explanation :-)  I still think the
>> version I suggested for this hunk is right though.
>
> I agree.  I now see what I had wrong.  Thanks for your patience and explanations.  If you like the wording I used in the doc and on immed_double_const, I think we have now resolved all issues.  The previous version was bootstraped and regression tested on darwin, fortran, c, c++, objective-c++...  I'll do one more run with the update for simplify_const_unary_operation, as that can trip when before it would merely return 0.
>
> Ok?
>
>
> Index: doc/rtl.texi
> ===================================================================
> --- doc/rtl.texi	(revision 185706)
> +++ doc/rtl.texi	(working copy)
> @@ -1479,8 +1479,13 @@ This type of expression represents the i
>  is customarily accessed with the macro @code{INTVAL} as in
>  @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
>  
> -Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
> -must be sign extended to full width (e.g., with @code{gen_int_mode}).
> +Constants generated for modes with fewer bits than in
> +@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
> +@code{gen_int_mode}).  For constants for modes with more bits than in
> +@code{HOST_WIDE_INT} the implied high order bits of that constant are
> +copies of the top bit, however values are neither signed nor unsigned.
> +All operations defined on such constants define the signededness.

I'm not someone who should be wordsmithing, but I think:

...copies of the top bit.  Note however that values are neither inherently
signed nor inherently unsigned; where necessary, signedness is determined
by the rtl operation instead.

> @@ -1510,7 +1515,12 @@ Represents either a floating-point const
>  integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
>  bits but small enough to fit within twice that number of bits (GCC
>  does not provide a mechanism to represent even larger constants).  In
> -the latter case, @var{m} will be @code{VOIDmode}.
> +the latter case, @var{m} will be @code{VOIDmode}.  For integral values
> +constants for modes with more bits than twice the number in
> +@code{HOST_WIDE_INT} the implied high order bits of that constant are
> +copies of the top bit of @code{CONST_DOUBLE_HIGH}, however integral
> +values are neither signed nor unsigned.  All operations defined on
> +such constants define the signededness.

Same idea here.

> +/* Return an rtx for the sum of X and the integer C, given that X has
> +   mode MODE.  This routine should be used instead of plus_constant
> +   when they want to ensure that addition happens in a particular
> +   mode, which is necessary when x can be a VOIDmode CONST_INT or

Sorry, just noticed, should be "...when X can be..."

Looks good to me otherwise, thanks.  Richi?

Richard

Re: remove wrong code in immed_double_const

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Mar 26, 2012, at 1:03 PM, Richard Sandiford wrote:
> I think:
> 
> ...copies of the top bit.  Note however that values are neither inherently
> signed nor inherently unsigned; where necessary, signedness is determined
> by the rtl operation instead.

Sounds good to me, changed.

> Same idea here.

Fixed.

>> +/* Return an rtx for the sum of X and the integer C, given that X has
>> +   mode MODE.  This routine should be used instead of plus_constant
>> +   when they want to ensure that addition happens in a particular
>> +   mode, which is necessary when x can be a VOIDmode CONST_INT or
> 
> Sorry, just noticed, should be "...when X can be..."

Fixed.

Richard has ok all his bits, now down to you for final ok.

Ok?


[-- Attachment #2: oi-trunk-2.diffs.txt --]
[-- Type: text/plain, Size: 14054 bytes --]

	* doc/rtl.texi (const_double): Document as sign-extending.
	* expmed.c (expand_mult): Ensure we don't use shift
	incorrectly.
	* emit-rtl.c (immed_double_int_const): Refine to state the
	value is signed.
	* simplify-rtx.c (mode_signbit_p): Add a fixme for wider than
	CONST_DOUBLE integers.
	(simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no
	negative values are converted.  Fix conversions bigger than
	HOST_BITS_PER_WIDE_INT.
	(simplify_binary_operation_1): Ensure we don't use shift
	incorrectly.
	(simplify_immed_subreg): Sign-extend CONST_DOUBLEs.
	* explow.c (plus_constant_mode): Add.
	(plus_constant): Implement with plus_constant_mode.
	* rtl.h (plus_constant_mode): Add.

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 185706)
+++ doc/rtl.texi	(working copy)
@@ -1479,8 +1479,19 @@ This type of expression represents the i
 is customarily accessed with the macro @code{INTVAL} as in
 @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
 
-Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
-must be sign extended to full width (e.g., with @code{gen_int_mode}).
+Constants generated for modes with fewer bits than in
+@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
+@code{gen_int_mode}).  For constants for modes with more bits than in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit.  Note however that values are neither
+inherently signed nor inherently unsigned; where necessary, signedness
+is determined by the rtl operation instead.
+
+
+, however values are neither signed nor unsigned.
+All operations defined on such constants define the signededness.
+
+
 
 @findex const0_rtx
 @findex const1_rtx
@@ -1510,7 +1521,13 @@ Represents either a floating-point const
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+constants for modes with more bits than twice the number in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit of @code{CONST_DOUBLE_HIGH}.  Note however that
+integral values are neither inherently signed nor inherently unsigned;
+where necessary, signedness is determined by the rtl operation
+instead.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
Index: expmed.c
===================================================================
--- expmed.c	(revision 185706)
+++ expmed.c	(working copy)
@@ -3135,8 +3135,10 @@ expand_mult (enum machine_mode mode, rtx
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   shift, target, unsignedp);
+	      if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1
+		  || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     shift, target, unsignedp);
 	    }
 	}
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 185706)
+++ emit-rtl.c	(working copy)
@@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en
 
 /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
    of ints: I0 is the low-order word and I1 is the high-order word.
-   Do not use this routine for non-integer modes; convert to
-   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
+   For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the
+   implied upper bits are copies of the high bit of i1.  The value
+   itself is neither signed nor unsigned.  Do not use this routine for
+   non-integer modes; convert to REAL_VALUE_TYPE and use
+   CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
 immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
@@ -531,10 +534,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
@@ -546,8 +548,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */
Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 185706)
+++ simplify-rtx.c	(working copy)
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, 
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1355,16 +1356,11 @@ simplify_const_unary_operation (enum rtx
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
-      else
+      if (op_mode == VOIDmode
+	  || GET_MODE_PRECISION (op_mode) > 2 * HOST_BITS_PER_WIDE_INT)
+	/* We should never get a negative number.  */
+	gcc_assert (hv >= 0);
+      else if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
       REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode);
@@ -1718,7 +1714,7 @@ simplify_const_unary_operation (enum rtx
   else if (GET_CODE (op) == CONST_DOUBLE
 	   && SCALAR_FLOAT_MODE_P (GET_MODE (op))
 	   && GET_MODE_CLASS (mode) == MODE_INT
-	   && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0)
+	   && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0)
     {
       /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX
 	 operators are intentionally left unspecified (to ease implementation
@@ -1783,7 +1779,7 @@ simplify_const_unary_operation (enum rtx
 	    return const0_rtx;
 
 	  /* Test against the unsigned upper bound.  */
-	  if (width == 2*HOST_BITS_PER_WIDE_INT)
+	  if (width == 2 * HOST_BITS_PER_WIDE_INT)
 	    {
 	      th = -1;
 	      tl = -1;
@@ -2380,7 +2376,9 @@ simplify_binary_operation_1 (enum rtx_co
 	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
 	  && GET_MODE (op0) == mode
 	  && CONST_DOUBLE_LOW (trueop1) == 0
-	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2 * HOST_BITS_PER_WIDE_INT - 1
+	      || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -5189,6 +5187,7 @@ simplify_immed_subreg (enum machine_mode
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -5201,10 +5200,11 @@ simplify_immed_subreg (enum machine_mode
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT - 1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
Index: explow.c
===================================================================
--- explow.c	(revision 185706)
+++ explow.c	(working copy)
@@ -74,14 +74,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enu
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when X can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is different from the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -91,12 +97,26 @@ plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -104,11 +124,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  /* Sorry, we have no way to represent overflows this wide.
+	     To fix, add constant support wider than CONST_DOUBLE.  */
+	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -120,10 +143,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
 	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
 	{
-	  tem
-	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
+	  tem = force_const_mem (GET_MODE (x), tem);
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -142,31 +163,17 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* The interesting case is adding the integer to a sum.
-	 Look for constant term in the sum and combine
-	 with C.  For an integer constant term, we make a combined
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* The interesting case is adding the integer to a sum.  Look
+	 for constant term in the sum and combine with C.  For an
+	 integer constant term or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -176,7 +183,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -196,6 +203,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* Return an rtx for the sum of X and the integer C.  */
+
+rtx
+plus_constant (rtx x, HOST_WIDE_INT c)
+{
+  return plus_constant_mode (GET_MODE (x), x, c);
+}
 \f
 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
Index: rtl.h
===================================================================
--- rtl.h	(revision 185706)
+++ rtl.h	(working copy)
@@ -1644,6 +1644,7 @@ extern int ceil_log2 (unsigned HOST_WIDE
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);

Re: remove wrong code in immed_double_const

[Mike Stump]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Mar 26, 2012, at 4:57 PM, Mike Stump wrote:
> On Mar 26, 2012, at 1:03 PM, Richard Sandiford wrote:
>> I think:
>> 
>> ...copies of the top bit.  Note however that values are neither inherently
>> signed nor inherently unsigned; where necessary, signedness is determined
>> by the rtl operation instead.
> 
> Sounds good to me, changed.

Oh, review caught one last problem:

> +, however values are neither signed nor unsigned.
> +All operations defined on such constants define the signededness.

This was edit cruft from the last rewording for the doc, the cruft has been removed.


[-- Attachment #2: oi-trunk-3.diffs.txt --]
[-- Type: text/plain, Size: 13922 bytes --]

	* doc/rtl.texi (const_double): Document as sign-extending.
	* expmed.c (expand_mult): Ensure we don't use shift
	incorrectly.
	* emit-rtl.c (immed_double_int_const): Refine to state the
	value is signed.
	* simplify-rtx.c (mode_signbit_p): Add a fixme for wider than
	CONST_DOUBLE integers.
	(simplify_const_unary_operation, UNSIGNED_FLOAT): Ensure no
	negative values are converted.  Fix conversions bigger than
	HOST_BITS_PER_WIDE_INT.
	(simplify_binary_operation_1): Ensure we don't use shift
	incorrectly.
	(simplify_immed_subreg): Sign-extend CONST_DOUBLEs.
	* explow.c (plus_constant_mode): Add.
	(plus_constant): Implement with plus_constant_mode.
	* rtl.h (plus_constant_mode): Add.

Index: doc/rtl.texi
===================================================================
--- doc/rtl.texi	(revision 186111)
+++ doc/rtl.texi	(working copy)
@@ -1479,8 +1479,13 @@ This type of expression represents the i
 is customarily accessed with the macro @code{INTVAL} as in
 @code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.
 
-Constants generated for modes with fewer bits than @code{HOST_WIDE_INT}
-must be sign extended to full width (e.g., with @code{gen_int_mode}).
+Constants generated for modes with fewer bits than in
+@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
+@code{gen_int_mode}).  For constants for modes with more bits than in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit.  Note however that values are neither
+inherently signed nor inherently unsigned; where necessary, signedness
+is determined by the rtl operation instead.
 
 @findex const0_rtx
 @findex const1_rtx
@@ -1510,7 +1515,13 @@ Represents either a floating-point const
 integer constant too large to fit into @code{HOST_BITS_PER_WIDE_INT}
 bits but small enough to fit within twice that number of bits (GCC
 does not provide a mechanism to represent even larger constants).  In
-the latter case, @var{m} will be @code{VOIDmode}.
+the latter case, @var{m} will be @code{VOIDmode}.  For integral values
+constants for modes with more bits than twice the number in
+@code{HOST_WIDE_INT} the implied high order bits of that constant are
+copies of the top bit of @code{CONST_DOUBLE_HIGH}.  Note however that
+integral values are neither inherently signed nor inherently unsigned;
+where necessary, signedness is determined by the rtl operation
+instead.
 
 @findex CONST_DOUBLE_LOW
 If @var{m} is @code{VOIDmode}, the bits of the value are stored in
Index: expmed.c
===================================================================
--- expmed.c	(revision 186111)
+++ expmed.c	(working copy)
@@ -3139,8 +3139,10 @@ expand_mult (enum machine_mode mode, rtx
 	    {
 	      int shift = floor_log2 (CONST_DOUBLE_HIGH (op1))
 			  + HOST_BITS_PER_WIDE_INT;
-	      return expand_shift (LSHIFT_EXPR, mode, op0,
-				   shift, target, unsignedp);
+	      if (shift < 2 * HOST_BITS_PER_WIDE_INT - 1
+		  || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT)
+		return expand_shift (LSHIFT_EXPR, mode, op0,
+				     shift, target, unsignedp);
 	    }
 	}
 
Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 186111)
+++ emit-rtl.c	(working copy)
@@ -517,8 +517,11 @@ immed_double_int_const (double_int i, en
 
 /* Return a CONST_DOUBLE or CONST_INT for a value specified as a pair
    of ints: I0 is the low-order word and I1 is the high-order word.
-   Do not use this routine for non-integer modes; convert to
-   REAL_VALUE_TYPE and use CONST_DOUBLE_FROM_REAL_VALUE.  */
+   For values that are larger than 2*HOST_BITS_PER_WIDE_INT, the
+   implied upper bits are copies of the high bit of i1.  The value
+   itself is neither signed nor unsigned.  Do not use this routine for
+   non-integer modes; convert to REAL_VALUE_TYPE and use
+   CONST_DOUBLE_FROM_REAL_VALUE.  */
 
 rtx
 immed_double_const (HOST_WIDE_INT i0, HOST_WIDE_INT i1, enum machine_mode mode)
@@ -531,10 +534,9 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
      1) If GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT, then we use
 	gen_int_mode.
-     2) GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT, but the value of
-	the integer fits into HOST_WIDE_INT anyway (i.e., i1 consists only
-	from copies of the sign bit, and sign of i0 and i1 are the same),  then
-	we return a CONST_INT for i0.
+     2) If the value of the integer fits into HOST_WIDE_INT anyway
+        (i.e., i1 consists only from copies of the sign bit, and sign
+	of i0 and i1 are the same), then we return a CONST_INT for i0.
      3) Otherwise, we create a CONST_DOUBLE for i0 and i1.  */
   if (mode != VOIDmode)
     {
@@ -546,8 +548,6 @@ immed_double_const (HOST_WIDE_INT i0, HO
 
       if (GET_MODE_BITSIZE (mode) <= HOST_BITS_PER_WIDE_INT)
 	return gen_int_mode (i0, mode);
-
-      gcc_assert (GET_MODE_BITSIZE (mode) == 2 * HOST_BITS_PER_WIDE_INT);
     }
 
   /* If this integer fits in one word, return a CONST_INT.  */
Index: simplify-rtx.c
===================================================================
--- simplify-rtx.c	(revision 186111)
+++ simplify-rtx.c	(working copy)
@@ -97,6 +97,7 @@ mode_signbit_p (enum machine_mode mode, 
       width -= HOST_BITS_PER_WIDE_INT;
     }
   else
+    /* FIXME: We don't yet have a representation for wider modes.  */
     return false;
 
   if (width < HOST_BITS_PER_WIDE_INT)
@@ -1355,16 +1356,11 @@ simplify_const_unary_operation (enum rtx
       else
 	lv = CONST_DOUBLE_LOW (op),  hv = CONST_DOUBLE_HIGH (op);
 
-      if (op_mode == VOIDmode)
-	{
-	  /* We don't know how to interpret negative-looking numbers in
-	     this case, so don't try to fold those.  */
-	  if (hv < 0)
-	    return 0;
-	}
-      else if (GET_MODE_PRECISION (op_mode) >= HOST_BITS_PER_WIDE_INT * 2)
-	;
-      else
+      if (op_mode == VOIDmode
+	  || GET_MODE_PRECISION (op_mode) > 2 * HOST_BITS_PER_WIDE_INT)
+	/* We should never get a negative number.  */
+	gcc_assert (hv >= 0);
+      else if (GET_MODE_PRECISION (op_mode) <= HOST_BITS_PER_WIDE_INT)
 	hv = 0, lv &= GET_MODE_MASK (op_mode);
 
       REAL_VALUE_FROM_UNSIGNED_INT (d, lv, hv, mode);
@@ -1718,7 +1714,7 @@ simplify_const_unary_operation (enum rtx
   else if (GET_CODE (op) == CONST_DOUBLE
 	   && SCALAR_FLOAT_MODE_P (GET_MODE (op))
 	   && GET_MODE_CLASS (mode) == MODE_INT
-	   && width <= 2*HOST_BITS_PER_WIDE_INT && width > 0)
+	   && width <= 2 * HOST_BITS_PER_WIDE_INT && width > 0)
     {
       /* Although the overflow semantics of RTL's FIX and UNSIGNED_FIX
 	 operators are intentionally left unspecified (to ease implementation
@@ -1783,7 +1779,7 @@ simplify_const_unary_operation (enum rtx
 	    return const0_rtx;
 
 	  /* Test against the unsigned upper bound.  */
-	  if (width == 2*HOST_BITS_PER_WIDE_INT)
+	  if (width == 2 * HOST_BITS_PER_WIDE_INT)
 	    {
 	      th = -1;
 	      tl = -1;
@@ -2380,7 +2376,9 @@ simplify_binary_operation_1 (enum rtx_co
 	      || GET_MODE_CLASS (GET_MODE (trueop1)) == MODE_INT)
 	  && GET_MODE (op0) == mode
 	  && CONST_DOUBLE_LOW (trueop1) == 0
-	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0)
+	  && (val = exact_log2 (CONST_DOUBLE_HIGH (trueop1))) >= 0
+	  && (val < 2 * HOST_BITS_PER_WIDE_INT - 1
+	      || GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT))
 	return simplify_gen_binary (ASHIFT, mode, op0,
 				    GEN_INT (val + HOST_BITS_PER_WIDE_INT));
 
@@ -5189,6 +5187,7 @@ simplify_immed_subreg (enum machine_mode
 	case CONST_DOUBLE:
 	  if (GET_MODE (el) == VOIDmode)
 	    {
+	      unsigned char extend = 0;
 	      /* If this triggers, someone should have generated a
 		 CONST_INT instead.  */
 	      gcc_assert (elem_bitsize > HOST_BITS_PER_WIDE_INT);
@@ -5201,10 +5200,11 @@ simplify_immed_subreg (enum machine_mode
 		    = CONST_DOUBLE_HIGH (el) >> (i - HOST_BITS_PER_WIDE_INT);
 		  i += value_bit;
 		}
-	      /* It shouldn't matter what's done here, so fill it with
-		 zero.  */
+
+	      if (CONST_DOUBLE_HIGH (el) >> (HOST_BITS_PER_WIDE_INT - 1))
+		extend = -1;
 	      for (; i < elem_bitsize; i += value_bit)
-		*vp++ = 0;
+		*vp++ = extend;
 	    }
 	  else
 	    {
Index: explow.c
===================================================================
--- explow.c	(revision 186111)
+++ explow.c	(working copy)
@@ -74,14 +74,20 @@ trunc_int_for_mode (HOST_WIDE_INT c, enu
   return c;
 }
 
-/* Return an rtx for the sum of X and the integer C.  */
+/* Return an rtx for the sum of X and the integer C, given that X has
+   mode MODE.  This routine should be used instead of plus_constant
+   when they want to ensure that addition happens in a particular
+   mode, which is necessary when X can be a VOIDmode CONST_INT or
+   CONST_DOUBLE and the width of the constant is different from the
+   width of the expression.  */
+/* TODO: All callers of plus_constant should migrate to this routine,
+   and once they do, we can assert that mode is not VOIDmode.  */
 
 rtx
-plus_constant (rtx x, HOST_WIDE_INT c)
+plus_constant_mode (enum machine_mode mode, rtx x, HOST_WIDE_INT c)
 {
   RTX_CODE code;
   rtx y;
-  enum machine_mode mode;
   rtx tem;
   int all_constant = 0;
 
@@ -91,12 +97,26 @@ plus_constant (rtx x, HOST_WIDE_INT c)
  restart:
 
   code = GET_CODE (x);
-  mode = GET_MODE (x);
   y = x;
 
   switch (code)
     {
     case CONST_INT:
+      if (GET_MODE_BITSIZE (mode) > HOST_BITS_PER_WIDE_INT)
+	{
+	  unsigned HOST_WIDE_INT l1 = INTVAL (x);
+	  HOST_WIDE_INT h1 = (l1 >> (HOST_BITS_PER_WIDE_INT - 1)) ? -1 : 0;
+	  unsigned HOST_WIDE_INT l2 = c;
+	  HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
+	  unsigned HOST_WIDE_INT lv;
+	  HOST_WIDE_INT hv;
+
+	  if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	    gcc_unreachable ();
+
+	  return immed_double_const (lv, hv, VOIDmode);
+	}
+
       return GEN_INT (INTVAL (x) + c);
 
     case CONST_DOUBLE:
@@ -104,11 +124,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	unsigned HOST_WIDE_INT l1 = CONST_DOUBLE_LOW (x);
 	HOST_WIDE_INT h1 = CONST_DOUBLE_HIGH (x);
 	unsigned HOST_WIDE_INT l2 = c;
-	HOST_WIDE_INT h2 = c < 0 ? ~0 : 0;
+	HOST_WIDE_INT h2 = c < 0 ? -1 : 0;
 	unsigned HOST_WIDE_INT lv;
 	HOST_WIDE_INT hv;
 
-	add_double (l1, h1, l2, h2, &lv, &hv);
+	if (add_double_with_sign (l1, h1, l2, h2, &lv, &hv, false))
+	  /* Sorry, we have no way to represent overflows this wide.
+	     To fix, add constant support wider than CONST_DOUBLE.  */
+	  gcc_assert (GET_MODE_BITSIZE (mode) <= 2 * HOST_BITS_PER_WIDE_INT);
 
 	return immed_double_const (lv, hv, VOIDmode);
       }
@@ -120,10 +143,8 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
 	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
 	{
-	  tem
-	    = force_const_mem (GET_MODE (x),
-			       plus_constant (get_pool_constant (XEXP (x, 0)),
-					      c));
+	  tem = plus_constant_mode (mode, get_pool_constant (XEXP (x, 0)), c);
+	  tem = force_const_mem (GET_MODE (x), tem);
 	  if (memory_address_p (GET_MODE (tem), XEXP (tem, 0)))
 	    return tem;
 	}
@@ -142,31 +163,17 @@ plus_constant (rtx x, HOST_WIDE_INT c)
       break;
 
     case PLUS:
-      /* The interesting case is adding the integer to a sum.
-	 Look for constant term in the sum and combine
-	 with C.  For an integer constant term, we make a combined
-	 integer.  For a constant term that is not an explicit integer,
-	 we cannot really combine, but group them together anyway.
-
-	 Restart or use a recursive call in case the remaining operand is
-	 something that we handle specially, such as a SYMBOL_REF.
+      /* The interesting case is adding the integer to a sum.  Look
+	 for constant term in the sum and combine with C.  For an
+	 integer constant term or a constant term that is not an
+	 explicit integer, we combine or group them together anyway.
 
 	 We may not immediately return from the recursive call here, lest
 	 all_constant gets lost.  */
 
-      if (CONST_INT_P (XEXP (x, 1)))
+      if (CONSTANT_P (XEXP (x, 1)))
 	{
-	  c += INTVAL (XEXP (x, 1));
-
-	  if (GET_MODE (x) != VOIDmode)
-	    c = trunc_int_for_mode (c, GET_MODE (x));
-
-	  x = XEXP (x, 0);
-	  goto restart;
-	}
-      else if (CONSTANT_P (XEXP (x, 1)))
-	{
-	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant (XEXP (x, 1), c));
+	  x = gen_rtx_PLUS (mode, XEXP (x, 0), plus_constant_mode (mode, XEXP (x, 1), c));
 	  c = 0;
 	}
       else if (find_constant_term_loc (&y))
@@ -176,7 +183,7 @@ plus_constant (rtx x, HOST_WIDE_INT c)
 	  rtx copy = copy_rtx (x);
 	  rtx *const_loc = find_constant_term_loc (&copy);
 
-	  *const_loc = plus_constant (*const_loc, c);
+	  *const_loc = plus_constant_mode (mode, *const_loc, c);
 	  x = copy;
 	  c = 0;
 	}
@@ -196,6 +203,14 @@ plus_constant (rtx x, HOST_WIDE_INT c)
   else
     return x;
 }
+
+/* Return an rtx for the sum of X and the integer C.  */
+
+rtx
+plus_constant (rtx x, HOST_WIDE_INT c)
+{
+  return plus_constant_mode (GET_MODE (x), x, c);
+}
 \f
 /* If X is a sum, return a new sum like X but lacking any constant terms.
    Add all the removed constant terms into *CONSTPTR.
Index: rtl.h
===================================================================
--- rtl.h	(revision 186111)
+++ rtl.h	(working copy)
@@ -1644,6 +1644,7 @@ extern int ceil_log2 (unsigned HOST_WIDE
 /* In explow.c */
 extern HOST_WIDE_INT trunc_int_for_mode	(HOST_WIDE_INT, enum machine_mode);
 extern rtx plus_constant (rtx, HOST_WIDE_INT);
+extern rtx plus_constant_mode (enum machine_mode, rtx, HOST_WIDE_INT);
 
 /* In rtl.c */
 extern rtx rtx_alloc_stat (RTX_CODE MEM_STAT_DECL);