Martin Sebor <mse...@gmail.com> writes:
> On 10/25/2017 03:31 PM, Richard Sandiford wrote:
>> Martin Sebor <mse...@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
