On Mon, Dec 2, 2013 at 2:48 PM, Richard Sandiford
<rdsandif...@googlemail.com> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>> On Sat, Nov 30, 2013 at 10:43 AM, Richard Sandiford
>> <rdsandif...@googlemail.com> wrote:
>>> So maybe two INTEGER_CST lengths weren't enough. Because bitsizetype
>>> can be offset_int-sized, wi::to_offset had a TYPE_PRECISION condition
>>> to pick the array length:
>>>
>>> template <int N>
>>> inline unsigned int
>>> wi::extended_tree <N>::get_len () const
>>> {
>>> if (N == MAX_BITSIZE_MODE_ANY_INT
>>> || N > TYPE_PRECISION (TREE_TYPE (m_t)))
>>> return TREE_INT_CST_EXT_NUNITS (m_t);
>>> else
>>> return TREE_INT_CST_NUNITS (m_t);
>>> }
>>>
>>> and this TYPE_PRECISION condition was relatively hot in
>>> get_ref_base_and_extent when compiling insn-recog.ii.
>>>
>>> Adding a third length for offset_int does seem to reduce the cost of
>>> the offset_int + to_offset addition.
>>>
>>> Tested on x86_64-linux-gnu. OK to install?
>>
>> Hmm, that's now getting a bit excessive ...
>
> Well, we access trees in three different ways, and we want all of them
> to be cheap, so having three fields in the tree seems pretty natural.
Hmm, I guess you are right :/
>> inline unsigned int
>> wi::extended_tree <N>::get_len () const
>> {
>> - if (N == MAX_BITSIZE_MODE_ANY_INT
>> - || N > TYPE_PRECISION (TREE_TYPE (m_t)))
>> + if (N == ADDR_MAX_PRECISION)
>> + return TREE_INT_CST_OFFSET_NUNITS (m_t);
>> + else if (N == MAX_BITSIZE_MODE_ANY_INT
>> + || N > TYPE_PRECISION (TREE_TYPE (m_t)))
>> return TREE_INT_CST_EXT_NUNITS (m_t);
>> else
>> return TREE_INT_CST_NUNITS (m_t);
>>
>> Shouldn't it be N >= TYPE_PRECISION () btw?
>
> No, TREE_INT_CST_NUNITS is for accessing the tree in TYPE_PRECISION
> and TREE_INT_CST_EXT_NUNITS is for accessing it in wider precisions.
>
> N is always >= TYPE_PRECISION here, from the assert in the constructor:
>
> gcc_checking_assert (TYPE_PRECISION (TREE_TYPE (t)) <= N);
>
> TREE_INT_CST_OFFSET_NUNITS is just a cached x ? TREE_INT_CST_NUNITS (m_t)
> : TREE_INT_CST_EXT_NUNITS (m_t) result for a particular precision.
>
>> Looking at the implementation it seems it would also work with
>>
>> return MIN (TREE_INT_CST_EXT_NUNITS (m_t), N / HOST_BITS_PER_WIDE_INT);
>
> Yeah, the MIN in the patch was probably bogus sorry. It only works
> if we can assume that no bitsizetype will have ADDR_MAX_PRECISION
> significant (non-sign) bits -- in particular that there's no such thing as
> an all-1s _unsigned_ bitsizetype. That might be true in practice given
> the way we use offsets, but it isn't safe to generalise that to all N.
>
> A safer form would be:
>
> if (ext_len > OFFSET_INT_ELTS)
> TREE_INT_CST_OFFSET_NUNITS (t) = len;
> else
> TREE_INT_CST_OFFSET_NUNITS (t) = ext_len;
>
> The reason the general form doesn't work for all N is because of the
> compressed representation. E.g. the example I gave a while ago about
> a 256-bit all-1s unsigned number being { -1 } as a 256-bit number and
> { -1, -1, -1, -1, 0 } as a 257+-bit number.
>
> But the point of the patch is to avoid any runtime checks here,
> so the TYPE_PRECISION case is never actually used now. I just kept
> it around for completeness, since we'd been using it successfully so far.
> I can put in a gcc_unreachable if you prefer.
Yeah, I'd prefer a gcc_unreachable and a comment.
Thanks,
Richard.
> Thanks,
> Richard
