Re: remove wrong code in immed_double_const

[Mike Stump <mikestump at comcast dot net>]

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.