On 9/17/20 8:02 PM, Martin Sebor wrote:
> On 9/17/20 4:36 AM, Aldy Hernandez via Gcc-patches wrote:
>> This is the irange storage class. It is used to allocate the minimum
>> amount of storage needed for a given irange. Storage is automatically
>> freed at destruction.
>>
>> It is meant for long term storage, as opposed to int_range_max which
>> is meant for intermediate temporary results on the stack.
>>
>> The general gist is:
>>
>> irange_pool pool;
>>
>> // Allocate an irange of 5 sub-ranges.
>> irange *p = pool.allocate (5);
>>
>> // Allocate an irange of 3 sub-ranges.
>> irange *q = pool.allocate (3);
>>
>> // Allocate an irange with as many sub-ranges as are currently
>> // used in "some_other_range".
>> irange *r = pool.allocate (some_other_range);
>
> I used this as an opportunity to learn more about the irange classes,
> so I have more to say than this little change alone might justify.
>
>>
>> OK?
>> Aldy
>> ---
>> gcc/value-range.h | 63 +++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 63 insertions(+)
>>
>> diff --git a/gcc/value-range.h b/gcc/value-range.h
>> index 8497791c7b3..88cb3075bf0 100644
>> --- a/gcc/value-range.h
>> +++ b/gcc/value-range.h
>> @@ -43,6 +43,7 @@ enum value_range_kind
>>
>> class irange
>> {
>> + friend class irange_pool;
>> public:
>> // In-place setters.
>> void set (tree, tree, value_range_kind = VR_RANGE);
>> @@ -619,4 +620,66 @@ vrp_val_min (const_tree type)
>> return NULL_TREE;
>> }
>>
>> +// This is the irange storage class. It is used to allocate the
>> +// minimum amount of storage needed for a given irange. Storage is
>> +// automatically freed at destruction.
>> +//
>> +// It is meant for long term storage, as opposed to int_range_max
>> +// which is meant for intermediate temporary results on the stack.
>> +
>> +class irange_pool
>> +{
>> +public:
>> + irange_pool ();
>> + ~irange_pool ();
>> + // Return a new range with NUM_PAIRS.
>> + irange *allocate (unsigned num_pairs);
>> + // Return a copy of SRC with the minimum amount of sub-ranges needed
>> + // to represent it.
>> + irange *allocate (const irange &src);
>> +private:
>> + struct obstack irange_obstack;
>> +};
>
> Since the class owns a resource, its copy ctor and assignment should
> either transfer it or copy it between instances, or be disabled if
> copying isn't intended.
But that would be silly. Who would ever think of copying the allocator
object? :-). But it makes perfect sense. I've disabled copy and
assignment.
>
> I don't know much about the obstack APIs but if it's anything like
> malloc/free or new/delete I'm guessing the class isn't meant to be
> copied or assigned. If that's correct, I would suggest to make it
> an error by making its copy ctor and copy assignment operator
> private or deleted, e.g., by making use of DISABLE_COPY_AND_ASSIGN.
>
> Otherwise, if the implicitly provided copy ctor and assignment work
> correctly, I'd suggest to add a comment to the class making it clear
> that copying and assignment are in fact safe.
>
>
>> +
>> +inline
>> +irange_pool::irange_pool ()
>> +{
>> + obstack_init (&irange_obstack);
>> +}
>> +
>> +inline
>> +irange_pool::~irange_pool ()
>> +{
>> + obstack_free (&irange_obstack, NULL);
>> +}
>> +
>> +// Return a new range with NUM_PAIRS.
>> +
>> +inline irange *
>> +irange_pool::allocate (unsigned num_pairs)
>> +{
>> + // Never allocate 0 pairs.
>> + // Don't allocate 1 either, or we get legacy value_range's.
>> + if (num_pairs < 2)
>> + num_pairs = 2;
>> +
>> + struct newir {
>> + irange range;
>> + tree mem[1];
>> + };
>> + struct newir *r
>> + = (struct newir *) obstack_alloc (&irange_obstack,
>> + sizeof (struct newir)
>> + + sizeof (tree) * 2 * (num_pairs - 1));
>> + return new ((irange *) r) irange (&(r->mem[0]), num_pairs);
>
> FWIW, it took me a minute before I understood what this dense code
> does. Rewriting it like this helped:
Yeah. We need the newir object to get the alignment right.
>
> size_t nbytes
> = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
>
> struct newir *r
> = (newir *) obstack_alloc (&irange_obstack, nbytes);
>
> return new (r) irange (r->mem, num_pairs);
Indeed. I find splitting things up easier to read. FWIW, the original
code had it split up, and then it got munged up in one of our many
iterations. I blame Andrew :).
>
> The non-member placement new takes a void* argument so the cast from
> r's type to irange* isn't necessary. &(r->mem[0]) is the same as
> r->mem which also reads a little clearer to me. In C++, the class-
Same here :).
> id ("struct" or "class") can be omitted when there's no ambiguity.
I always thought common usage was to put the struct, but avoid the
class. I'm fine either way though.
>
> The allocated memory is passed to the irange ctor uninitialized but
> the ctor doesn't access it, and (AFAICS) neither do any other irange
> members, so that's fine. I like that the irange class is safe to
> use even when the array it manages is uninitialized. In fact, it
> might be a helpful comment to add to the irange and int_range ctors:
> that the array of trees passed to irange doesn't have to be
> initialized and the classes work correctly and treat a newly default
> constructed range as empty (undefined_p() is true).
That's true by design. Ranges start their life as the empty set.
I've incorporated your suggestions and some of David's below.
Thanks.
Aldy
This is the irange storage class. It is used to allocate the
minimum amount of storage needed for a given irange. Storage is
automatically freed at destruction of the storage class.
It is meant for long term storage, as opposed to int_range_max
which is meant for intermediate temporary results on the stack.
The general gist is:
irange_pool pool;
// Allocate an irange of 5 sub-ranges.
irange *p = pool.allocate (5);
// Allocate an irange of 3 sub-ranges.
irange *q = pool.allocate (3);
// Allocate an irange with as many sub-ranges as are currently
// used in "some_other_range".
irange *r = pool.allocate (some_other_range);
---
gcc/value-range.h | 65 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/gcc/value-range.h b/gcc/value-range.h
index 8497791c7b3..a4b5df4f89d 100644
--- a/gcc/value-range.h
+++ b/gcc/value-range.h
@@ -43,6 +43,7 @@ enum value_range_kind
class irange
{
+ friend class irange_pool;
public:
// In-place setters.
void set (tree, tree, value_range_kind = VR_RANGE);
@@ -619,4 +620,68 @@ vrp_val_min (const_tree type)
return NULL_TREE;
}
+// This is the irange storage class. It is used to allocate the
+// minimum amount of storage needed for a given irange. Storage is
+// automatically freed at destruction of the storage class.
+//
+// It is meant for long term storage, as opposed to int_range_max
+// which is meant for intermediate temporary results on the stack.
+//
+// The newly allocated irange is initialized to the empty set
+// (undefined_p() is true).
+
+class irange_pool
+{
+public:
+ irange_pool ();
+ ~irange_pool ();
+ // Return a new range with NUM_PAIRS.
+ irange *allocate (unsigned num_pairs);
+ // Return a copy of SRC with the minimum amount of sub-ranges needed
+ // to represent it.
+ irange *allocate (const irange &src);
+private:
+ DISABLE_COPY_AND_ASSIGN (irange_pool);
+ struct obstack m_obstack;
+};
+
+inline
+irange_pool::irange_pool ()
+{
+ obstack_init (&m_obstack);
+}
+
+inline
+irange_pool::~irange_pool ()
+{
+ obstack_free (&m_obstack, NULL);
+}
+
+// Return a new range with NUM_PAIRS.
+
+inline irange *
+irange_pool::allocate (unsigned num_pairs)
+{
+ // Never allocate 0 pairs.
+ // Don't allocate 1 either, or we get legacy value_range's.
+ if (num_pairs < 2)
+ num_pairs = 2;
+
+ struct newir {
+ irange range;
+ tree mem[1];
+ };
+ size_t nbytes = (sizeof (newir) + sizeof (tree) * 2 * (num_pairs - 1));
+ struct newir *r = (newir *) obstack_alloc (&m_obstack, nbytes);
+ return new (r) irange (r->mem, num_pairs);
+}
+
+inline irange *
+irange_pool::allocate (const irange &src)
+{
+ irange *r = allocate (src.num_pairs ());
+ *r = src;
+ return r;
+}
+
#endif // GCC_VALUE_RANGE_H
--
2.26.2
