remove wrong code in immed_double_const

This removes some wrong code.

Ok?

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.  */
>

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.

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

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.

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

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.

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