Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor (was: Expensive selftests)
Hi!
On 2021-08-17T10:57:36+0200, Richard Biener wrote:
> On Tue, Aug 17, 2021 at 8:40 AM Thomas Schwinge
> wrote:
>> On 2021-08-16T14:10:00-0600, Martin Sebor wrote:
>> > On 8/16/21 6:44 AM, Thomas Schwinge wrote:
>> >> [...], to document the current behavior, I propose to
>> >> "Add more self-tests for 'hash_map' with Value type with non-trivial
>> >> constructor/destructor", see attached. OK to push to master branch?
>> >> (Also cherry-pick into release branches, eventually?)
>> > Adding more tests sounds like an excellent idea. I'm not sure about
>> > the idea of adding loopy selftests that iterate as many times as in
>> > the patch (looks like 1234 times two?)
>>
>> Correct, and I agree it's a sensible concern, generally.
>>
>> The current 1234 times two iterations is really arbitrary (should
>> document that in the test case), just so that we trigger a few hash table
>> expansions.
>
> You could lower N_init (the default init is just 13!),
> even with just 128 inserted elements you'll trigger
> expansions to 31, 61 and 127 elements.
> I think the test is OK but it's also reasonable to lower
> the '1234' times and add a comment as to the count should
> trigger hashtable expansions "a few times".
Did that as follows:
/* Verify basic construction and destruction of Value objects. */
{
/* Configure, arbitrary. */
const size_t N_init = 0;
const int N_elem = 28;
..., and:
/* Verify aspects of 'hash_table::expand'. */
static void
test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline)
{
/* Configure, so that hash table expansion triggers a few times. */
const size_t N_init = 0;
const int N_elem = 70;
size_t expand_c_expected = 4;
[...]
if (expand)
{
++expand_c;
[...]
ASSERT_EQ (expand_c, expand_c_expected);
Given that it's all deterministic (as far as I can tell...), we may
verify an exact number of hash table expansions.
Pushed "Add more self-tests for 'hash_map' with Value type with
non-trivial constructor/destructor" to master branch in commit
e4f16e9f357a38ec702fb69a0ffab9d292a6af9b, see attached.
Grüße
Thomas
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
>From e4f16e9f357a38ec702fb69a0ffab9d292a6af9b Mon Sep 17 00:00:00 2001
From: Thomas Schwinge
Date: Fri, 13 Aug 2021 17:53:12 +0200
Subject: [PATCH] Add more self-tests for 'hash_map' with Value type with
non-trivial constructor/destructor
... to document the current behavior.
gcc/
* hash-map-tests.c (test_map_of_type_with_ctor_and_dtor): Extend.
(test_map_of_type_with_ctor_and_dtor_expand): Add function.
(hash_map_tests_c_tests): Call it.
---
gcc/hash-map-tests.c | 152 +++
1 file changed, 152 insertions(+)
diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c
index 5b6b192cd28..257f2be0c26 100644
--- a/gcc/hash-map-tests.c
+++ b/gcc/hash-map-tests.c
@@ -278,6 +278,156 @@ test_map_of_type_with_ctor_and_dtor ()
ASSERT_TRUE (val_t::ndefault + val_t::ncopy == val_t::ndtor);
}
+
+
+ /* Verify basic construction and destruction of Value objects. */
+ {
+/* Configure, arbitrary. */
+const size_t N_init = 0;
+const int N_elem = 28;
+
+void *a[N_elem];
+for (size_t i = 0; i < N_elem; ++i)
+ a[i] = &a[i];
+
+val_t::ndefault = 0;
+val_t::ncopy = 0;
+val_t::nassign = 0;
+val_t::ndtor = 0;
+Map m (N_init);
+ASSERT_EQ (val_t::ndefault
+ + val_t::ncopy
+ + val_t::nassign
+ + val_t::ndtor, 0);
+
+for (int i = 0; i < N_elem; ++i)
+ {
+ m.get_or_insert (a[i]);
+ ASSERT_EQ (val_t::ndefault, 1 + i);
+ ASSERT_EQ (val_t::ncopy, 0);
+ ASSERT_EQ (val_t::nassign, 0);
+ ASSERT_EQ (val_t::ndtor, i);
+
+ m.remove (a[i]);
+ ASSERT_EQ (val_t::ndefault, 1 + i);
+ ASSERT_EQ (val_t::ncopy, 0);
+ ASSERT_EQ (val_t::nassign, 0);
+ ASSERT_EQ (val_t::ndtor, 1 + i);
+ }
+ }
+}
+
+/* Verify aspects of 'hash_table::expand'. */
+
+static void
+test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline)
+{
+ /* Configure, so that hash table expansion triggers a few times. */
+ const size_t N_init = 0;
+ const int N_elem = 70;
+ size_t expand_c_expected = 4;
+ size_t expand_c = 0;
+
+ void *a[N_elem];
+ for (size_t i = 0; i < N_elem; ++i)
+a[i] = &a[i];
+
+ typedef hash_map Map;
+
+ /* Note that we are starting with a fresh 'Map'. Even if an existing one has
+ been cleared out completely, there remain 'deleted' elements, and these
+ would disturb the following logic, where we don't have access to the
+ actual 'm_n_deleted' value. */
+ size_t m_n_deleted = 0;
+
+ val_t::ndefault = 0;
+ val_t::ncopy = 0;
+ val_t::nassign = 0;
+ val_t::ndtor = 0;
+
Re: Expensive selftests (was: 'hash_map>')
This causes a bootstrap failure for me. PR/101959 On Tue, Aug 17, 2021 at 5:00 AM Richard Biener via Gcc wrote: > > On Tue, Aug 17, 2021 at 8:40 AM Thomas Schwinge > wrote: > > > > Hi! > > > > On 2021-08-16T14:10:00-0600, Martin Sebor wrote: > > > On 8/16/21 6:44 AM, Thomas Schwinge wrote: > > >> [...], to document the current behavior, I propose to > > >> "Add more self-tests for 'hash_map' with Value type with non-trivial > > >> constructor/destructor", see attached. OK to push to master branch? > > >> (Also cherry-pick into release branches, eventually?) > > > > (Attached again, for easy reference.) > > > > > Adding more tests sounds like an excellent idea. I'm not sure about > > > the idea of adding loopy selftests that iterate as many times as in > > > the patch (looks like 1234 times two?) > > > > Correct, and I agree it's a sensible concern, generally. > > > > The current 1234 times two iterations is really arbitrary (should > > document that in the test case), just so that we trigger a few hash table > > expansions. > > You could lower N_init (the default init is just 13!), > even with just 128 inserted elements you'll trigger > expansions to 31, 61 and 127 elements. > > > For 'selftest-c', we've got originally: > > > > -fself-test: 74775 pass(es) in 0.309299 seconds > > -fself-test: 74775 pass(es) in 0.366041 seconds > > -fself-test: 74775 pass(es) in 0.356663 seconds > > -fself-test: 74775 pass(es) in 0.355009 seconds > > -fself-test: 74775 pass(es) in 0.367575 seconds > > -fself-test: 74775 pass(es) in 0.320406 seconds > > > > ..., and with my changes we've got: > > > > -fself-test: 94519 pass(es) in 0.327755 seconds > > -fself-test: 94519 pass(es) in 0.369522 seconds > > -fself-test: 94519 pass(es) in 0.355531 seconds > > -fself-test: 94519 pass(es) in 0.362179 seconds > > -fself-test: 94519 pass(es) in 0.363176 seconds > > -fself-test: 94519 pass(es) in 0.318930 seconds > > > > So it really seems to be all in the noise? > > Yes. I think the test is OK but it's also reasonable to lower > the '1234' times and add a comment as to the count should > trigger hashtable expansions "a few times". > > Richard. > > > Yet: > > > > > Selftests run each time GCC > > > builds (i.e., even during day to day development). It seems to me > > > that it might be better to run such selftests only as part of > > > the bootstrap process. > > > > I'd rather have thought about a '--param self-test-expensive' (or > > similar), and then invoke the selftests via a new > > 'gcc/testsuite/selftests/expensive.exp' (or similar). > > > > Or, adapt 'gcc/testsuite/gcc.dg/plugin/expensive_selftests_plugin.c', > > that is, invoke them via the GCC plugin mechanism, which also seems to be > > easy enough? > > > > I don't have a strong opinion about where/when these tests get run, so > > will happily take any suggestions. > > > > > > Grüße > > Thomas > > > > > > - > > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, > > 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: > > Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; > > Registergericht München, HRB 106955
Re: [RFC] Adding a new attribute to function param to mark it as constant
On 8/18/21 12:52 AM, Prathamesh Kulkarni wrote:
On Fri, 13 Aug 2021 at 22:44, Martin Sebor wrote:
On 8/12/21 2:32 AM, Prathamesh Kulkarni wrote:
On Sat, 7 Aug 2021 at 02:09, Martin Sebor wrote:
On 8/6/21 4:51 AM, Richard Earnshaw wrote:
On 06/08/2021 01:06, Martin Sebor via Gcc wrote:
On 8/4/21 3:46 AM, Richard Earnshaw wrote:
On 03/08/2021 18:44, Martin Sebor wrote:
On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote:
On Tue, 27 Jul 2021 at 13:49, Richard Biener
wrote:
On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc
wrote:
On Fri, 23 Jul 2021 at 23:29, Andrew Pinski
wrote:
On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc
wrote:
Hi,
Continuing from this thread,
https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html
The proposal is to provide a mechanism to mark a parameter in a
function as a literal constant.
Motivation:
Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h:
__extension__ extern __inline int32x2_t
__attribute__ ((__always_inline__, __gnu_inline__,
__artificial__))
vshl_n_s32 (int32x2_t __a, const int __b)
{
return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b);
}
and it's caller:
int32x2_t f (int32x2_t x)
{
return vshl_n_s32 (x, 1);
}
Can't you do similar to what is done already in the aarch64
back-end:
#define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0]))
#define __AARCH64_LANE_CHECK(__vec, __idx) \
__builtin_aarch64_im_lane_boundsi (sizeof(__vec),
sizeof(__vec[0]), __idx)
?
Yes this is about lanes but you could even add one for min/max
which
is generic and such; add an argument to say the intrinsics name
even.
You could do this as a non-target builtin if you want and reuse it
also for the aarch64 backend.
Hi Andrew,
Thanks for the suggestions. IIUC, we could use this approach to
check
if the argument
falls within a certain range (min / max), but I am not sure how it
will help to determine
if the arg is a constant immediate ? AFAIK, vshl_n intrinsics
require
that the 2nd arg is immediate ?
Even the current RTL builtin checking is not consistent across
optimization levels:
For eg:
int32x2_t f(int32_t *restrict a)
{
int32x2_t v = vld1_s32 (a);
int b = 2;
return vshl_n_s32 (v, b);
}
With pristine trunk, compiling with -O2 results in no errors because
constant propagation replaces 'b' with 2, and during expansion,
expand_builtin_args is happy. But at -O0, it results in the error -
"argument 2 must be a constant immediate".
So I guess we need some mechanism to mark a parameter as a
constant ?
I guess you want to mark it in a way that the frontend should force
constant evaluation and error if that's not possible? C++ doesn't
allow to declare a parameter as 'constexpr' but something like
void foo (consteval int i);
since I guess you do want to allow passing constexpr arguments
in C++ or in C extended forms of constants like
static const int a[4];
foo (a[1]);
? But yes, this looks useful to me.
Hi Richard,
Thanks for the suggestions and sorry for late response.
I have attached a prototype patch that implements consteval attribute.
As implemented, the attribute takes at least one argument(s), which
refer to parameter position,
and the corresponding parameter must be const qualified, failing
which, the attribute is ignored.
I'm curious why the argument must be const-qualified. If it's
to keep it from being changed in ways that would prevent it from
being evaluated at compile-time in the body of the function then
to be effective, the enforcement of the constraint should be on
the definition of the function. Otherwise, the const qualifier
could be used in a declaration of a function but left out from
a subsequent definition of it, letting it modify it, like so:
__attribute__ ((consteval (1))) void f (const int);
inline __attribute__ ((always_inline)) void f (int i) { ++i; }
In this particular case it's because the inline function is
implementing an intrinsic operation in the architecture and the
instruction only supports a literal constant value. At present we
catch this while trying to expand the intrinsic, but that can lead to
poor diagnostics because we really want to report against the line of
code calling the intrinsic.
Presumably the intrinsics can accept (or can be made to accept) any
constant integer expressions, not just literals. E.g., the aarch64
builtin below accepts them. For example, this is accepted in C++:
__Int64x2_t void f (__Int32x2_t a)
{
constexpr int n = 2;
return __builtin_aarch64_vshll_nv2si (a, n + 1);
}
Making the intrinscis accept constant arguments in constexpr-like
functions and introducing a constexpr-lite attribute (for C code)
was what I was suggesting bythe constexpr comment below. I'd find
that a much more general and more powerful design.
Yes, it would be unfortunate if the rule made it impossible to avoid
idiomatic const-exprs like those you would write in C
Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959]
Hi! On 2021-08-18T09:35:25-0400, David Edelsohn wrote: > This causes a bootstrap failure for me. > > PR/101959 Sorry for that; "details". ;-| OK to push the attached "Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959]", which does resolve the issue for a '-m32' i686-pc-linux-gnu build? Grüße Thomas > On Tue, Aug 17, 2021 at 5:00 AM Richard Biener via Gcc > wrote: >> >> On Tue, Aug 17, 2021 at 8:40 AM Thomas Schwinge >> wrote: >> > >> > Hi! >> > >> > On 2021-08-16T14:10:00-0600, Martin Sebor wrote: >> > > On 8/16/21 6:44 AM, Thomas Schwinge wrote: >> > >> [...], to document the current behavior, I propose to >> > >> "Add more self-tests for 'hash_map' with Value type with non-trivial >> > >> constructor/destructor", see attached. OK to push to master branch? >> > >> (Also cherry-pick into release branches, eventually?) >> > >> > (Attached again, for easy reference.) >> > >> > > Adding more tests sounds like an excellent idea. I'm not sure about >> > > the idea of adding loopy selftests that iterate as many times as in >> > > the patch (looks like 1234 times two?) >> > >> > Correct, and I agree it's a sensible concern, generally. >> > >> > The current 1234 times two iterations is really arbitrary (should >> > document that in the test case), just so that we trigger a few hash table >> > expansions. >> >> You could lower N_init (the default init is just 13!), >> even with just 128 inserted elements you'll trigger >> expansions to 31, 61 and 127 elements. >> >> > For 'selftest-c', we've got originally: >> > >> > -fself-test: 74775 pass(es) in 0.309299 seconds >> > -fself-test: 74775 pass(es) in 0.366041 seconds >> > -fself-test: 74775 pass(es) in 0.356663 seconds >> > -fself-test: 74775 pass(es) in 0.355009 seconds >> > -fself-test: 74775 pass(es) in 0.367575 seconds >> > -fself-test: 74775 pass(es) in 0.320406 seconds >> > >> > ..., and with my changes we've got: >> > >> > -fself-test: 94519 pass(es) in 0.327755 seconds >> > -fself-test: 94519 pass(es) in 0.369522 seconds >> > -fself-test: 94519 pass(es) in 0.355531 seconds >> > -fself-test: 94519 pass(es) in 0.362179 seconds >> > -fself-test: 94519 pass(es) in 0.363176 seconds >> > -fself-test: 94519 pass(es) in 0.318930 seconds >> > >> > So it really seems to be all in the noise? >> >> Yes. I think the test is OK but it's also reasonable to lower >> the '1234' times and add a comment as to the count should >> trigger hashtable expansions "a few times". >> >> Richard. >> >> > Yet: >> > >> > > Selftests run each time GCC >> > > builds (i.e., even during day to day development). It seems to me >> > > that it might be better to run such selftests only as part of >> > > the bootstrap process. >> > >> > I'd rather have thought about a '--param self-test-expensive' (or >> > similar), and then invoke the selftests via a new >> > 'gcc/testsuite/selftests/expensive.exp' (or similar). >> > >> > Or, adapt 'gcc/testsuite/gcc.dg/plugin/expensive_selftests_plugin.c', >> > that is, invoke them via the GCC plugin mechanism, which also seems to be >> > easy enough? >> > >> > I don't have a strong opinion about where/when these tests get run, so >> > will happily take any suggestions. >> > >> > >> > Grüße >> > Thomas >> > >> > >> > - >> > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, >> > 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: >> > Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; >> > Registergericht München, HRB 106955 - Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 >From a6075945f392a93fa03eaf163cbe34fc5cfe8fe1 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge Date: Wed, 18 Aug 2021 17:20:40 +0200 Subject: [PATCH] Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959] Bug fix for recent commit e4f16e9f357a38ec702fb69a0ffab9d292a6af9b "Add more self-tests for 'hash_map' with Value type with non-trivial constructor/destructor". gcc/ PR bootstrap/101959 * hash-map-tests.c (test_map_of_type_with_ctor_and_dtor_expand): Use an 'int_hash'. --- gcc/hash-map-tests.c | 17 ++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/gcc/hash-map-tests.c b/gcc/hash-map-tests.c index 257f2be0c26..6acc0d4337e 100644 --- a/gcc/hash-map-tests.c +++ b/gcc/hash-map-tests.c @@ -328,11 +328,22 @@ test_map_of_type_with_ctor_and_dtor_expand (bool remove_some_inline) size_t expand_c_expected = 4; size_t expand_c = 0; - void *a[N_elem]; + /* For stability of this testing, we need all Key values 'k' to produce + unique hash values 'Traits::hash (k)', as otherwise the dynamic
Re: Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' work on 32-bit architectures [PR101959]
On August 18, 2021 5:34:28 PM GMT+02:00, Thomas Schwinge wrote: >Hi! > >On 2021-08-18T09:35:25-0400, David Edelsohn wrote: >> This causes a bootstrap failure for me. >> >> PR/101959 > >Sorry for that; "details". ;-| OK to push the attached >"Make 'gcc/hash-map-tests.c:test_map_of_type_with_ctor_and_dtor_expand' >work on 32-bit architectures [PR101959]", which does resolve the issue >for a '-m32' i686-pc-linux-gnu build? Ok. Richard. > >Grüße > Thomas > > >> On Tue, Aug 17, 2021 at 5:00 AM Richard Biener via Gcc >> wrote: >>> >>> On Tue, Aug 17, 2021 at 8:40 AM Thomas Schwinge >>> wrote: >>> > >>> > Hi! >>> > >>> > On 2021-08-16T14:10:00-0600, Martin Sebor wrote: >>> > > On 8/16/21 6:44 AM, Thomas Schwinge wrote: >>> > >> [...], to document the current behavior, I propose to >>> > >> "Add more self-tests for 'hash_map' with Value type with non-trivial >>> > >> constructor/destructor", see attached. OK to push to master branch? >>> > >> (Also cherry-pick into release branches, eventually?) >>> > >>> > (Attached again, for easy reference.) >>> > >>> > > Adding more tests sounds like an excellent idea. I'm not sure about >>> > > the idea of adding loopy selftests that iterate as many times as in >>> > > the patch (looks like 1234 times two?) >>> > >>> > Correct, and I agree it's a sensible concern, generally. >>> > >>> > The current 1234 times two iterations is really arbitrary (should >>> > document that in the test case), just so that we trigger a few hash table >>> > expansions. >>> >>> You could lower N_init (the default init is just 13!), >>> even with just 128 inserted elements you'll trigger >>> expansions to 31, 61 and 127 elements. >>> >>> > For 'selftest-c', we've got originally: >>> > >>> > -fself-test: 74775 pass(es) in 0.309299 seconds >>> > -fself-test: 74775 pass(es) in 0.366041 seconds >>> > -fself-test: 74775 pass(es) in 0.356663 seconds >>> > -fself-test: 74775 pass(es) in 0.355009 seconds >>> > -fself-test: 74775 pass(es) in 0.367575 seconds >>> > -fself-test: 74775 pass(es) in 0.320406 seconds >>> > >>> > ..., and with my changes we've got: >>> > >>> > -fself-test: 94519 pass(es) in 0.327755 seconds >>> > -fself-test: 94519 pass(es) in 0.369522 seconds >>> > -fself-test: 94519 pass(es) in 0.355531 seconds >>> > -fself-test: 94519 pass(es) in 0.362179 seconds >>> > -fself-test: 94519 pass(es) in 0.363176 seconds >>> > -fself-test: 94519 pass(es) in 0.318930 seconds >>> > >>> > So it really seems to be all in the noise? >>> >>> Yes. I think the test is OK but it's also reasonable to lower >>> the '1234' times and add a comment as to the count should >>> trigger hashtable expansions "a few times". >>> >>> Richard. >>> >>> > Yet: >>> > >>> > > Selftests run each time GCC >>> > > builds (i.e., even during day to day development). It seems to me >>> > > that it might be better to run such selftests only as part of >>> > > the bootstrap process. >>> > >>> > I'd rather have thought about a '--param self-test-expensive' (or >>> > similar), and then invoke the selftests via a new >>> > 'gcc/testsuite/selftests/expensive.exp' (or similar). >>> > >>> > Or, adapt 'gcc/testsuite/gcc.dg/plugin/expensive_selftests_plugin.c', >>> > that is, invoke them via the GCC plugin mechanism, which also seems to be >>> > easy enough? >>> > >>> > I don't have a strong opinion about where/when these tests get run, so >>> > will happily take any suggestions. >>> > >>> > >>> > Grüße >>> > Thomas >>> > >>> > >>> > - >>> > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, >>> > 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: >>> > Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; >>> > Registergericht München, HRB 106955 > > >- >Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 >München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas >Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht >München, HRB 106955
