On 8/6/20 6:30 PM, Jonathan Wakely wrote:
On 06/08/20 16:17 +0200, Aldy Hernandez wrote:
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);
| ^
Can we just avoid using offsetof there?
Untested ...
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -1281,8 +1281,8 @@ template<typename T, typename A>
inline size_t
vec<T, A, vl_embed>::embedded_size (unsigned alloc)
{
- typedef vec<T, A, vl_embed> vec_embedded;
- return offsetof (vec_embedded, m_vecdata) + alloc * sizeof (T);
+ size_t offset = (char*)&m_vecdata - (char*)this;
+ return offset + alloc * sizeof (T);
}
Now we have:
In file included from /home/aldyh/src/gcc/gcc/rtl.h:30,
from /home/aldyh/src/gcc/gcc/genpreds.c:27:
/home/aldyh/src/gcc/gcc/vec.h: In static member function ‘static size_t
vec<T, A, vl_embed>::embedded_size(unsigned int)’:
/home/aldyh/src/gcc/gcc/vec.h:1284:27: error: invalid use of member
‘vec<T, A, vl_embed>::m_vecdata’ in static member function
1284 | size_t offset = (char*)&m_vecdata - (char*)this;
| ^~~~~~~~~
/home/aldyh/src/gcc/gcc/vec.h:626:5: note: declared here
626 | T m_vecdata[1];
| ^~~~~~~~~
/home/aldyh/src/gcc/gcc/vec.h:1284:46: error: ‘this’ is unavailable for
static member functions
1284 | size_t offset = (char*)&m_vecdata - (char*)this;
| ^~~~
