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 () { }
};
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).
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.)
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. :-)
Efficiency is certainly important, but it doesn't have to come
at the expense of usability or correctness. I think it's possible
(and important) to design interfaces that are usable safely and
intuitively, and difficult to misuse, while also accommodating
advanced efficient use cases.
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).
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.
Martin
