This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: remove wrong code in immed_double_const
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.