On 8/6/20 12:48 PM, Jonathan Wakely wrote:
On 06/08/20 12:31 +0200, Richard Biener wrote:
On Thu, Aug 6, 2020 at 12:19 PM Jonathan Wakely <jwak...@redhat.com>
wrote:
On 06/08/20 06:16 +0100, Richard Sandiford wrote:
>Andrew MacLeod via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> On 8/5/20 12:54 PM, Richard Biener via Gcc-patches wrote:
>>> On August 5, 2020 5:09:19 PM GMT+02:00, Martin Jambor
<mjam...@suse.cz> wrote:
>>>> On Fri, Jul 31 2020, Aldy Hernandez via Gcc-patches wrote:
>>>> [...]
>>>>
>>>>> * ipa-cp changes from vec<value_range> to std::vec<value_range>.
>>>>>
>>>>> We are using std::vec to ensure constructors are run, which they
>>>> aren't
>>>>> in our internal vec<> implementation. Although we usually
steer away
>>>>> from using std::vec because of interactions with our GC system,
>>>>> ipcp_param_lattices is only live within the pass and allocated
with
>>>> calloc.
>>>> Ummm... I did not object but I will save the URL of this message
in the
>>>> archive so that I can waive it in front of anyone complaining why I
>>>> don't use our internal vec's in IPA data structures.
>>>>
>>>> But it actually raises a broader question: was this supposed to
be an
>>>> exception, allowed only not to complicate the irange patch
further, or
>>>> will this be generally accepted thing to do when someone wants
to have
>>>> a
>>>> vector of constructed items?
>>> It's definitely not what we want. You have to find another
solution to this problem.
>>>
>>> Richard.
>>>
>>
>> Why isn't it what we want?
>>
>> This is a small vector local to the pass so it doesn't interfere with
>> our PITA GTY.
>> The class is pretty straightforward, but we do need a constructor to
>> initialize the pointer and the max-size field. There is no
allocation
>> done per element, so a small number of elements have a couple of
fields
>> initialized per element. We'd have to loop to do that anyway.
>>
>> GCC's vec<> does not provide he ability to run a constructor,
std::vec
>> does.
>
>I realise you weren't claiming otherwise, but: that could be fixed :-)
It really should be.
Artificial limitations like that are just a booby trap for the unwary.
It's probably also historic because we couldn't even implement
the case of re-allocation correctly without std::move, could we?
I don't see why not. std::vector worked fine without std::move, it's
just more efficient with std::move, and can be used with a wider set
of element types.
When reallocating you can just copy each element to the new storage
and destroy the old element. If your type is non-copyable then you
need std::move, but I don't think the types I see used with vec<> are
non-copyable. Most of them are trivially-copyable.
I think the benefit of std::move to GCC is likely to be permitting
cheap copies to be made where previously they were banned for
performance reasons, but not because those copies were impossible.
For the record, neither value_range nor int_range<N> require any
allocations. The sub-range storage resides in the stack or wherever it
was defined. However, it is definitely not a POD.
Digging deeper, I notice that the original issue that caused us to use
std::vector was not in-place new but the safe_grow_cleared. The
original code had:
auto_vec<value_range, 32> known_value_ranges;
...
...
if (!vr.undefined_p () && !vr.varying_p ())
{
if (!known_value_ranges.length ())
known_value_ranges.safe_grow_cleared (count);
known_value_ranges[i] = vr;
}
I would've gladly kept the auto_vec, had I been able to do call the
constructor by doing an in-place new:
if (!vr.undefined_p () && !vr.varying_p ())
{
if (!known_value_ranges.length ())
- known_value_ranges.safe_grow_cleared (count);
+ {
+ known_value_ranges.safe_grow_cleared (count);
+ for (int i = 0; i < count; ++i)
+ new (&known_value_ranges[i]) value_range ();
+ }
known_value_ranges[i] = vr;
}
}
But alas, compiling yields:
In file included from /usr/include/wchar.h:35,
from /usr/include/c++/10/cwchar:44,
from /usr/include/c++/10/bits/postypes.h:40,
from /usr/include/c++/10/iosfwd:40,
from /usr/include/gmp-x86_64.h:34,
from /usr/include/gmp.h:59,
from /home/aldyh/src/gcc/gcc/system.h:687,
from /home/aldyh/src/gcc/gcc/ipa-fnsummary.c:55:
/home/aldyh/src/gcc/gcc/vec.h: In instantiation of ‘static size_t vec<T, A,
vl_embed>::embedded_size(unsigned int) [with T = int_range<1>; A = va_heap; size_t
= long unsigned int]’:
/home/aldyh/src/gcc/gcc/vec.h:288:58: required from ‘static void va_heap::reserve(vec<T,
va_heap, vl_embed>*&, unsigned int, bool) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1746:20: required from ‘bool vec<T>::reserve(unsigned
int, bool) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1766:10: required from ‘bool
vec<T>::reserve_exact(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1894:3: required from ‘void
vec<T>::safe_grow(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1912:3: required from ‘void
vec<T>::safe_grow_cleared(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/ipa-fnsummary.c:634:51: required from here
/home/aldyh/src/gcc/gcc/vec.h:1285:20: warning: ‘offsetof’ within non-standard-layout type
‘vec_embedded’ {aka ‘vec<int_range<1>, va_heap, vl_embed>’} is
conditionally-supported [-Winvalid-offsetof]
1285 | return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T);
| ^
The attached patch reverts the functionality to auto_vec<>, but the
in-place new doesn't work. Does anyone have any suggestions?
Aldy
commit b508fee91fd473cdc82c7c6c404cd09cf06b5af7
Author: Aldy Hernandez <al...@redhat.com>
Date: Thu Aug 6 15:32:28 2020 +0200
in-place new on resized elements.
fails with:
g++ -fno-PIE -c -g -DIN_GCC -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wno-error=format-diag -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -I. -I. -I/home/aldyh/src/gcc/gcc -I/home/aldyh/src/gcc/gcc/. -I/home/aldyh/src/gcc/gcc/../include -I/home/aldyh/src/gcc/gcc/../libcpp/include -I/home/aldyh/src/gcc/gcc/../libdecnumber -I/home/aldyh/src/gcc/gcc/../libdecnumber/bid -I../libdecnumber -I/home/aldyh/src/gcc/gcc/../libbacktrace -o ipa-fnsummary.o -MT ipa-fnsummary.o -MMD -MP -MF ./.deps/ipa-fnsummary.TPo /home/aldyh/src/gcc/gcc/ipa-fnsummary.c
In file included from /usr/include/wchar.h:35,
from /usr/include/c++/10/cwchar:44,
from /usr/include/c++/10/bits/postypes.h:40,
from /usr/include/c++/10/iosfwd:40,
from /usr/include/gmp-x86_64.h:34,
from /usr/include/gmp.h:59,
from /home/aldyh/src/gcc/gcc/system.h:687,
from /home/aldyh/src/gcc/gcc/ipa-fnsummary.c:55:
/home/aldyh/src/gcc/gcc/vec.h: In instantiation of ‘static size_t vec<T, A, vl_embed>::embedded_size(unsigned int) [with T = int_range<1>; A = va_heap; size_t = long unsigned int]’:
/home/aldyh/src/gcc/gcc/vec.h:288:58: required from ‘static void va_heap::reserve(vec<T, va_heap, vl_embed>*&, unsigned int, bool) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1746:20: required from ‘bool vec<T>::reserve(unsigned int, bool) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1766:10: required from ‘bool vec<T>::reserve_exact(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1894:3: required from ‘void vec<T>::safe_grow(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/vec.h:1912:3: required from ‘void vec<T>::safe_grow_cleared(unsigned int) [with T = int_range<1>]’
/home/aldyh/src/gcc/gcc/ipa-fnsummary.c:634:51: required from here
/home/aldyh/src/gcc/gcc/vec.h:1285:20: warning: ‘offsetof’ within non-standard-layout type ‘vec_embedded’ {aka ‘vec<int_range<1>, va_heap, vl_embed>’} is conditionally-supported [-Winvalid-offsetof]
1285 | return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T);
| ^
diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 49bab04524b..4c6bc9f1877 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -82,7 +82,6 @@ along with GCC; see the file COPYING3. If not see
#include "gimplify.h"
#include "stringpool.h"
#include "attribs.h"
-#include <vector>
#include "tree-into-ssa.h"
/* Summaries. */
@@ -331,7 +330,7 @@ static void
evaluate_conditions_for_known_args (struct cgraph_node *node,
bool inline_p,
vec<tree> known_vals,
- const std::vector<value_range> &known_value_ranges,
+ vec<value_range> known_value_ranges,
vec<ipa_agg_value_set> known_aggs,
clause_t *ret_clause,
clause_t *ret_nonspec_clause)
@@ -446,7 +445,7 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
continue;
}
}
- if (c->operand_num < (int) known_value_ranges.size ()
+ if (c->operand_num < (int) known_value_ranges.length ()
&& !c->agg_contents
&& !known_value_ranges[c->operand_num].undefined_p ()
&& !known_value_ranges[c->operand_num].varying_p ()
@@ -555,7 +554,7 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
{
struct cgraph_node *callee = e->callee->ultimate_alias_target ();
class ipa_fn_summary *info = ipa_fn_summaries->get (callee);
- std::vector<value_range> known_value_ranges (32);
+ auto_vec<value_range, 32> known_value_ranges;
class ipa_edge_args *args;
if (clause_ptr)
@@ -630,11 +629,11 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool inline_p,
i));
if (!vr.undefined_p () && !vr.varying_p ())
{
- if (!known_value_ranges.size ())
+ if (!known_value_ranges.length ())
{
- known_value_ranges.resize (count);
+ known_value_ranges.safe_grow_cleared (count);
for (int i = 0; i < count; ++i)
- known_value_ranges[i].set_undefined ();
+ new (&known_value_ranges[i]) value_range ();
}
known_value_ranges[i] = vr;
}
@@ -808,7 +807,7 @@ ipa_fn_summary_t::duplicate (cgraph_node *src,
}
evaluate_conditions_for_known_args (dst, false,
known_vals,
- std::vector<value_range> (),
+ vNULL,
vNULL,
&possible_truths,
/* We are going to specialize,
@@ -3692,8 +3691,7 @@ estimate_ipcp_clone_size_and_time (struct cgraph_node *node,
clause_t clause, nonspec_clause;
/* TODO: Also pass known value ranges. */
- evaluate_conditions_for_known_args (node, false, known_vals,
- std::vector<value_range> (),
+ evaluate_conditions_for_known_args (node, false, known_vals, vNULL,
known_aggs, &clause, &nonspec_clause);
ipa_call_context ctx (node, clause, nonspec_clause,
known_vals, known_contexts,
