Re: [006/nnn] poly_int: tree constants

[Richard Sandiford <richard.sandiford@linaro.org>]

Martin Sebor <msebor@gmail.com> writes:
> On 10/25/2017 03:31 PM, Richard Sandiford wrote:
>> Martin Sebor <msebor@gmail.com> writes:
>>> On 10/23/2017 11:00 AM, Richard Sandiford wrote:
>>>> +#if NUM_POLY_INT_COEFFS == 1
>>>> +extern inline __attribute__ ((__gnu_inline__)) poly_int64
>>>> +tree_to_poly_int64 (const_tree t)
>>>
>>> I'm curious about the extern inline and __gnu_inline__ here and
>>> not in poly_int_tree_p below.  Am I correct in assuming that
>>> the combination is a holdover from the days when GCC was compiled
>>> using a C compiler, and that the way to write the same definition
>>> in C++ 98 is simply:
>>>
>>>    inline poly_int64
>>>    tree_to_poly_int64 (const_tree t)
>>>
>>>> +{
>>>> +  gcc_assert (tree_fits_poly_int64_p (t));
>>>> +  return TREE_INT_CST_LOW (t);
>>>> +}
>>>
>>> If yes, I would suggest to use the C++ form (and at some point,
>>> changing the existing uses of the GCC/C idiom to the C++ form
>>> as well).
>>>
>>> Otherwise, if something requires the use of the C form I would
>>> suggest to add a brief comment explaining it.
>>
>> You probably saw that this is based on tree_to_[su]hwi.  AIUI the
>> differences from plain C++ inline are that:
>>
>> a) with __gnu_inline__, an out-of-line definition must still exist.
>>    That fits this use case well, because the inline is conditional on
>>    the #ifdef and tree.c has an out-of-line definition either way.
>>    If we used normal inline, we'd need to add extra #ifs to tree.c
>>    as well, to avoid multiple definitions.
>>
>> b) __gnu_inline__ has the strength of __always_inline__, but without the
>>    correctness implications if inlining is impossible for any reason.
>>    I did try normal inline first, but it wasn't strong enough.  The
>>    compiler ended up measurably faster if I copied the tree_to_[su]hwi
>>    approach.
>
> Thanks for the clarification.  I'm not sure I fully understand
> it but I'm happy to take your word for it that's necessary.  I
> would just recommend adding a brief comment to this effect since
> it isn't obvious.
>
>>>> +
>>>> +inline bool
>>>> +poly_int_tree_p (const_tree t, poly_int64_pod *value)
>>>> +{
>>> ...
>>
>> [This one is unconditionally inline.]
>>
>>>>  /* The tree and const_tree overload templates.   */
>>>>  namespace wi
>>>>  {
>>>> +  class unextended_tree
>>>> +  {
>>>> +  private:
>>>> +    const_tree m_t;
>>>> +
>>>> +  public:
>>>> +    unextended_tree () {}
>>>
>>> Defining no-op ctors is quite dangerous and error-prone.  I suggest
>>> to instead default initialize the member(s):
>>>
>>>    unextended_tree (): m_t () {}
>>>
>>> Ditto everywhere else, such as in:
>>
>> This is really performance-senesitive code though, so I don't think
>> we want to add any unnecessary initialisation.  Primitive types are
>> uninitalised by default too, and the point of this class is to
>> provide an integer-like interface.
>
> I understand the performance concern (more on that below), but
> to clarify the usability issues,  I don't think the analogy with
> primitive types is quite fitting here: int() evaluates to zero,
> as do the values of i and a[0] and a[1] after an object of type
> S is constructed using its default ctor, i.e., S ():
>
>    struct S {
>      int i;
>      int a[2];
>
>      S (): i (), a () { }
>    };

Sure, I realise that.  I meant that:

  int x;

doesn't initialise x to zero.  So it's a question of which case is the
most motivating one: using "x ()" to initialise x to 0 in a constructor
or "int x;" to declare a variable of type x, uninitialised.  I think the
latter use case is much more common (at least in GCC).  Rearranging
things, I said later:

>> In your other message you used the example of explicit default
>> initialisation, such as:
>>
>> class foo
>> {
>>   foo () : x () {}
>>   unextended_tree x;
>> };
>>
>> But I think we should strongly discourage that kind of thing.
>> If someone wants to initialise x to a particular value, like
>> integer_zero_node, then it would be better to do it explicitly.
>> If they don't care what the initial value is, then for these
>> integer-mimicing classes, uninitialised is as good as anything
>> else. :-)

What I meant was: if you want to initialise "i" to 1 in your example,
you'd have to write "i (1)".  Being able to write "i ()" instead of
"i (0)" saves one character but I don't think it adds much clarity.
Explicitly initialising something only seems worthwhile if you say
what you're initialising it to.

> With the new (and some existing) classes that's not so, and it
> makes them harder and more error-prone to use (I just recently
> learned this the hard way about offset_int and the debugging
> experience is still fresh in my memory).

Sorry about the bad experience.  But that kind of thing cuts
both ways.  If I write:

poly_int64
foo (void)
{
  poly_int64 x;
  x += 2;
  return x;
}

then I get a warning about x being used uninitialised, without
having had to run anything.  If we add default initialisation
then this becomes something that has to be debugged against
a particular test case, i.e. we've stopped the compiler from
giving us useful static analysis.

> When the cor is inline and the initialization unnecessary then
> GCC will in most instances eliminate it, so I also don't think
> the suggested change would have a significant impact on
> the efficiency of optimized code, but...
>
> ...if it is thought essential to provide a no-op ctor, I would
> suggest to consider making its property explicit, e.g., like so:
>
>    struct unextended_tree {
>
>      struct Uninit { };
>
>      // ...
>      unextended_tree (Uninit) { /* no initialization */ }
>      // ...
>    };
>
> This way the programmer has to explicitly opt in to using the
> unsafe ctor.  (This ctor is suitable for single objects, not
> arrays of such things, but presumably that would be sufficient.
> If not, there are tricks to make that work too.)

The default constructors for unextended_tree and extended_tree
are only there for the array case (in poly-int.h).

Part of the problem here is that we still have to live by C++03
POD rules.  If we moved to C++11, the need for the poly_int_pod/
poly_int split would go away and things would probably be much
simpler. :-)

[...]

>> Note that for this class NULL_TREE is not a safe default value.
>> The same goes for the wide-int.h classes, where a length or precision
>> of 0 is undefined and isn't necessarily going to be handled gracefully
>> or predictably.
>
> For offset_int both precision and length are known so I think
> it would make sense to have the default ctor value-initialize
> the object.  For wide_int, it seems to me that choosing some
> default precision and length in the default ctor would still
> be preferable to leaving the members indeterminate.  (That
> functionality could still be provided by some other ctor as
> I suggested above).

But which precision though?  If we pick a commonly-used one
then we make a missing initialisation bug very data-dependent.
Even if we pick a rarely-used one, we create a bug in which
the wide_int has the wrong precision even though all assignments
to it "obviously" have the right precision.

>>>>    template <int N>
>>>>    class extended_tree
>>>>    {
>>>> @@ -5139,11 +5225,13 @@ extern bool anon_aggrname_p (const_tree)
>>>>      const_tree m_t;
>>>>
>>>>    public:
>>>> +    extended_tree () {}
>>>>      extended_tree (const_tree);
>>> ...
>>>> Index: gcc/tree.c
>>>> ===================================================================
>>> ...
>>>> +
>>>> +/* Return true if T holds a polynomial pointer difference, storing it in
>>>> +   *VALUE if so.  A true return means that T's precision is no greater
>>>> +   than 64 bits, which is the largest address space we support, so *VALUE
>>>> +   never loses precision.  However, the signedness of the result is
>>>> +   somewhat arbitrary, since if B lives near the end of a 64-bit address
>>>> +   range and A lives near the beginning, B - A is a large positive value
>>>> +   outside the range of int64_t.  A - B is likewise a large negative value
>>>> +   outside the range of int64_t.  All the pointer difference really
>>>> +   gives is a raw pointer-sized bitstring that can be added to the first
>>>> +   pointer value to get the second.  */
>>>
>>> I'm not sure I understand the comment about the sign correctly, but
>>> if I do, I don't think it's correct.
>>>
>>> Because their difference wouldn't representable in any basic integer
>>> type (i.,e., in ptrdiff_t) the pointers described above could never
>>> point to the same object (or array), and so their difference is not
>>> meaningful.  C/C++ only define the semantics of a difference between
>>> pointers to the same object.  That restricts the size of the largest
>>> possible object typically to SIZE_MAX / 2, or at most SIZE_MAX on
>>> the handful of targets where ptrdiff_t has greater precision than
>>> size_t.  But even on those targets, the difference between any two
>>> pointers to the same object must be representable in ptrdiff_t,
>>> including the sign.
>>
>> But does that apply even when no pointer difference of that size
>> occurs in the original source?  I.e., is:
>>
>>   char *x = malloc (0x80000001)
>>
>> undefined in itself on 32-bit targets?
>
> No, the call itself isn't undefined, but it shouldn't succeed
> on a conforming implementation where ptrdiff_t is a 32-bit type
> (which is why GCC diagnoses it).  If the call were to succeed
> then  pointers to the allocated object would fail to meet the
> C requirements on additive operators.
>
>> Does it become undefined after:
>>
>>   for (unsigned int i = 0; i < 0x80000001; ++i)
>>     x[i++] = 0;
>>
>> where no large pointer difference is calculated?  But I realise
>> gcc's support for this kind of thing is limited, and that we do
>> try to emit a diagnostic for obvious instances...
>
> Yes, this is undefined, both in C (unless ptrdiff_t is wider
> than 32 bits) and in GCC, because x[0x80000000] doesn't refer
> to the 2147483648-th element of x.
>
>> In the (two) places that need this -- both conversions from
>> cst_and_fits_in_hwi -- the immediate problem is that the sign
>> of the type doesn't necessarily match the logical sign of the
>> difference.  E.g. a negative offset can be represented as a large
>> unsigned value of sizetype.
>
> I only meant to suggest that the comment be reworded so as
> not to imply that such pointers (that are farther apart than
> PTRDIFF_MAX) can point to the same object and be subtracted.

OK, how about:

/* Return true if T holds a polynomial pointer difference, storing it in
   *VALUE if so.  A true return means that T's precision is no greater
   than 64 bits, which is the largest address space we support, so *VALUE
   never loses precision.  However, the signedness of the result does
   not necessarily match the signedness of T: sometimes an unsigned type
   like sizetype is used to encode a value that is actually negative.  */

Thanks,
Richard