Segher Boessenkool <seg...@kernel.crashing.org> writes:
> On Wed, Jun 02, 2021 at 06:07:28PM +0100, Richard Sandiford wrote:
>> Segher Boessenkool <seg...@kernel.crashing.org> writes:
>> > Since times immemorial there has been const_int_rtx for all values from
>> > -64 to 64, but only constm1_rtx..const2_rtx have been available for
>> > convenient use. Change this, so that we can use all values in
>> > {-64,...,64} in RTL easily. This matters, because then we we just say
>> > if (XEXP (x, 1) == const16_rtx)
>> > and things like that, since all const_int in that range are unique. We
>> > already do for -1, 0, 1, 2, but we could for everything.
>
>> No strong objection, but personally I'd rather not add something
>> that is very specific to VOIDmode CONST_INTs. I realise it's very
>> unlikely that we'll ever be able to give CONST_INTs their proper mode
>> (no-one has the kind of time needed to do that), but I don't think we
>> should make the switch actively harder either.
>
> How does this make that harder?
>
> Having no mode for CONST_INTs makes some things significantly *easier*
> btw. Well you know that, that is what makes any conversion away from
> this so much harder :-)
Sure, most things have advantages and disadvantages. The same was
true for cc0: it made some things easier for ports.
> We have has const0_rtx etc. since forever, this patch just increases the
> range (to those values that have had guaranteed unique RTXes since
> decades as well).
But it will change the preferred way of writing something. If we do
this, then whenever any future patch happens to use an integer in the
expanded range, it will be a legitimate review comment to say that
you should use const31_rtx instead of GEN_INT (31), or should use
x == const31_rtx instead of testing the contents of x. If instead
the constant is outside [-64, 64] then the code should do whatever it
would have done before. This is something new that people will have
to remember. The current range of [-64, 64] might have been around for
a long time, but it's not something that people have had to internalise
until now.
IMO the range of cached integers is an implementation detail that could
in principle change in future based on profiling. It's not something
that we should hard-code into the interface to this extent.
And like Jeff says, at least const{m1,0,1,2}_rtx have generic
macros CONST{M1,0,1,2}_RTX that work for non-integer modes.
Adding constN_rtx macros that have no CONSTN_RTX version is new ground.
The (existing) constN_rtx names aren't great because they don't
make it clear that the rtx is specifically for scalar integers.
>> How about adding a new inline helper function that tests whether an
>> rtx is a CONST_INT with a given value? Then we could have a
>> __builtin_constant_p shortcut for the [-64, 64] case. That would
>> also avoid hard-coding the range.
>
> Currently you have to write the example as
> if (XEXP (x, 1) == const_int_rtx[MAX_SAVED_CONST_INT + 16])
> and with your suggestion it will be
> if (is_const_int_with_value (XEXP (x, 1), 16))
Why the long function name though? IMO:
if (is_const_int (XEXP (x, 1), 16))
would be fine. Maybe even:
if (is_int (XEXP (x, 1), 16))
since GEN_INT, gen_int_mode and (U)INTVAL also drop the “const_”.
> I like
> if (XEXP (x, 1) == const16_rtx)
> better: it is shorter and clearer (often you have something like this is
> more complex conditionals, it matters), and this pattern is already very
> well known (for -1, 0, 1, 2).
Well, it depends on the context. These days the preferred way of
writing GET_CODE (x) == CONST_INT is CONST_INT_P (x), so the new test
would typically be replacing another function-like test. It seems
inconsistent to say that CONST_INT_P (x) is more readable than a ==
condition when testing for specific codes, but == is more readable
than a predicate when testing for specific integer rtxes.
> Do you like this patch a bit better if I also add such an
> is_const_int_with_value function?
Not really, sorry. :-)
But anyway, I've said my piece. Let's see whether anyone else is
prepared to approve it.
Thanks,
Richard
