Re: patch to fix constant math -5th patch, rtl

[Richard Biener <richard.guenther@gmail.com>]

On Fri, May 3, 2013 at 2:37 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>>> See e.g. the hoops that cselib has to jump through:
>>>
>>> /* We need to pass down the mode of constants through the hash table
>>>    functions.  For that purpose, wrap them in a CONST of the appropriate
>>>    mode.  */
>>> static rtx
>>> wrap_constant (enum machine_mode mode, rtx x)
>>> {
>>>   if ((!CONST_SCALAR_INT_P (x)) && GET_CODE (x) != CONST_FIXED)
>>>     return x;
>>>   gcc_assert (mode != VOIDmode);
>>>   return gen_rtx_CONST (mode, x);
>>> }
>>>
>>> That is, cselib locally converts (const_int X) into (const:M (const_int X)),
>>> purely so that it doesn't lose track of the CONST_INT's mode.
>>> (const:M (const_int ...)) is invalid rtl elsewhere, but a necessary
>>> hack here all the same.
>>
>> Indeed ugly.  But I wonder why cselib needs to store constants in
>> hashtables at all ... they should be VALUEs themselves.  So the fix
>> for the above might not necessarily be to assign the CONST_INT
>> a mode (not that CONST_WIDE_INT would fix the above).
>
> I don't understand.  Do you mean that cselib values ought to have
> a field to say whether the value is constant or not, and if so, what
> constant that is?  That feels like just the same kind of hack as the above.
> The current idea of chaining all known equivalent rtxes in a list seems
> more natural than having a list of all known equivalent rtxes except
> CONST_INT and CONST_DOUBLE, which have to be stored separately instead.
> (Again, we have runtime constants like SYMBOL_REF, which store modes,
> and which would presumably still be in the normal rtx list.)
>
> CONST_WIDE_INT was never supposed to solve this problem.  I'm just giving
> it as an example to back up the argument that rtx constants do in fact
> have modes (although those modes are not stored in the rtx).  The code
> above is there to make sure that equivalence stays transitive.
> Without it we could have bogus equivalences like:
>
>   (A) (reg:DI X) == (const_int Y) == (reg:SI Z)
>
> even though it cannot be the case that:
>
>   (B) (reg:DI X) == (reg:SI Z)
>
> My point is that, semantically, (A) did not come from X and Z being
> equivalent to the "same" constant.  X was equivalent to (const_int:DI Y)
> and Z was equivalent to (const_int:SI Y).  (A) only came about because
> we happen to use the same rtx object to represent those two semantically-
> distinct constants.
>
> The idea isn't to make CONST_WIDE_INT get rid of the code above.
> The idea is to make sure that wide_int has a precision and so doesn't
> require code like the above to be written when dealing with wide_ints.
>
> In other words, I got the impression your argument was "the fact
> that CONST_INT and CONST_DOUBLE don't store a mode shows that
> wide_int shouldn't store a precision".  But the fact that CONST_INT
> and CONST_DOUBLE don't store a mode doesn't mean they don't _have_
> a mode.  You just have to keep track of that mode separately.
> And the same would apply to wide_int if we did the same thing there.
>
> What I was trying to argue was that storing the mode/precision
> separately is not always easy.  It's also much less robust,
> because getting the wrong mode or precision will only show up
> for certain values.  If the precision is stored in the wide_int,
> mismatches can be asserted for based on precision alone, regardless
> of the value.

I was just arguing that pointing out facts in the RTL land doesn't
necessarily influence wide-int which is purely separate.  So if you
argue that having a mode in RTL constants would be soo nice and
thus that is why you want a precision in wide-int then I don't follow
that argument.  If you want a mode in RTL constants then get a mode
in RTL constants!

This would make it immediately obvious where to get the precision
for wide-ints - something you do not address at all (and as you don't
I sort of cannot believe the 'it would be so nice to have a mode on RTL
constants').

That said, if modes on RTL constants were so useful then why not
have them on CONST_WIDE_INT at least?  Please.  Only sticking
them to wide-int in form of a precision is completely backward to me
(and I still think the core wide-int shouldn't have a precision, and if
you really want a wide-int-with-precision simply derive from wide-int).

>> Ok, so please then make all CONST_INTs and CONST_DOUBLEs have
>> a mode!
>
> I'm saying that CONST_INT and CONST_DOUBLE already have a mode, but that
> mode is not stored in the rtx.  So if you're saying "make all CONST_INTs
> and CONST_DOUBLEs _store_ a mode", then yeah, I'd like to :-)  But I see
> Kenny's patch as a prerequisite for that, because it consolidates the
> CONST_INT and CONST_DOUBLE code so that the choice of rtx code is
> less special.  Lots more work is needed after that.

If there were a separate patch consolidating the paths I'd be all for
doing that.
I don't see a reason that this cannot be done even with the current
code using double-ints.

> Although TBH, the huge pushback that Kenny has got from this patch
> puts me off ever trying that change.

Well.  The patch does so much together and is so large that makes
it basically unreviewable (or very hard to review at least).

> But storing the mode in the rtx is orthogonal to what Kenny is doing.
> The mode of each rtx constant is already available in the places
> that Kenny is changing, because we already do the work to keep track
> of the mode separately.  Being able to get the mode directly from the
> rtx would be simpler and IMO better, but the semantics are the same
> either way.

Well, you showed examples where it is impossible to get at the mode.

> Kenny's patch is not designed to "fix" the CONST_INT representation
> (although the patch does make it easier to "fix" the representation
> in future).  Kenny's patch is about representing and handling constants
> that we can't at the moment.

No, it is about much more.

> The argument isn't whether CONST_WIDE_INT repeats "mistakes" made for
> CONST_INT and CONST_DOUBLE; I hope we agree that CONST_WIDE_INT should
> behave like the other two, whatever that is.  The argument is about
> whether we copy the "mistake" into the wide_int class.

I don't see how CONST_WIDE_INT is in any way related to wide_int other
than that you use wide_int to operate on the constants encoded in
CONST_WIDE_INT.  As you have a mode available at the point you
create a wide_int from a CONST_WIDE_INT you can very easily just
use that modes precision to specify the precision of an operation
(or zero/sign-extend the result).  That's what happens hidden in the
wide-int implementation currently, but in the awkward way that allows
precision mismatches and leads to odd things like having a wide-int
1 constant with a precision.

> Storing a precision in wide_int in no way requires CONST_WIDE_INT
> to store a mode.  They are separate choices.

Yes.  And I obviously would have chosed to store a mode in CONST_WIDE_INT
and no precision in wide_int.  And I cannot see a good reason to
do it the way you did it ;)

>> The solution is not to have a CONST_WIDE_INT (again with VOIDmode
>> and no precision in the RTX object(!)) and only have wide_int have a
>> precision.
>
> Why is having a VOIDmode CONST_WIDE_INT any worse than having
> a VOIDmode CONST_INT or CONST_DOUBLE?  In all three cases the mode
> is being obtained/inferred from the same external source.

Well, we're arguing in circles - the argument that VOIDmode CONST_INT/DOUBLE
are bad is yours.  And if that's not bad I can't see why it is bad for wide-int
to not have a mode (or precision).

Richard.

> Richard