Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
Hi, On Mon, 12 Nov 2018, Martin Liška wrote: > > There's no fundamental reason why we can't poison identifiers in other > > headers. Indeed we do in vec.h. So move the whole thing including > > poisoning to hash-table.h? > > That's not feasible as gcc/gcc/genhooks.c files use the function and > we don't want to include hash-table.h in the generator files. gencfn-macros.c:#include "hash-table.h" genmatch.c:#include "hash-table.h" gentarget-def.c:#include "hash-table.h" So there's precedent. The other solution would be to ignore genhooks.c (i.e. let it continue using the non-typesafe variant), I'm not very worried about wrong uses creeping in there. It had like one material change in the last seven years. I think I prefer the latter (ignoring the problem). > So it's question whether it worth doing that? Jumping through hoops for generator files seems useless. But the general idea for your type-checking hashers for the compiler proper does seem useful. Ciao, Michael.
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
On 11/9/18 2:28 PM, Michael Matz wrote:
> Hi,
>
> On Thu, 8 Nov 2018, Martin Liška wrote:
>
>>> That seems better. But still, why declare this in system.h? It seems
>>> hash-table.h seems more appropriate.
>>
>> I need to declare it before I'll poison it. As system.h is included very
>> early, one can guarantee that there will be no usage before the
>> poisoning happens.
>
> Yes but it's also included everywhere, so adding anything to it comes at a
> cost, and conceptually it simply doesn't belong there.
Agree.
>
> There's no fundamental reason why we can't poison identifiers in other
> headers. Indeed we do in vec.h. So move the whole thing including
> poisoning to hash-table.h?
That's not feasible as gcc/gcc/genhooks.c files use the function and
we don't want to include hash-table.h in the generator files.
So second candidate can be gcc/hash-traits.h, but it's also not working:
/home/marxin/Programming/gcc/gcc/hash-traits.h:270:17: error:
‘gt_pointer_operator’ has not been declared
pch_nx (T , gt_pointer_operator op, void *cookie)
^~~
so we should eventually come up with "hash.h" and include it in many places as
there's following usage
in hash-traits.h:
212 inline hashval_t
213 string_hash::hash (const char *id)
214 {
215return hash_string (id);
216 }
So it's question whether it worth doing that?
Martin
>
>
> Ciao,
> Michael.
>
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
Hi, On Thu, 8 Nov 2018, Martin Liška wrote: > > That seems better. But still, why declare this in system.h? It seems > > hash-table.h seems more appropriate. > > I need to declare it before I'll poison it. As system.h is included very > early, one can guarantee that there will be no usage before the > poisoning happens. Yes but it's also included everywhere, so adding anything to it comes at a cost, and conceptually it simply doesn't belong there. There's no fundamental reason why we can't poison identifiers in other headers. Indeed we do in vec.h. So move the whole thing including poisoning to hash-table.h? Ciao, Michael.
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
On 11/8/18 2:15 PM, Michael Matz wrote: > Hi, > > On Wed, 7 Nov 2018, Martin Liška wrote: > >>> Whereever the new function belongs it certainly isn't system.h. Also >>> the definition in a header seems excessive. Sure, it enables inlining >>> of it, but that seems premature optimization. It contains a loop, and >>> inlining anything with loops that aren't very likely to loop just once >>> or never just blows code for no gain. Also as the function is leaf >>> there won't be any second-order effect from inlining. >> >> Ok, works for me. As you know my main motivation was to provide stronger >> type declaration that can be used for 'const char *'. So what about >> providing 2 wrappers and poisoning the implementation? Hi. > > That seems better. But still, why declare this in system.h? It seems > hash-table.h seems more appropriate. I need to declare it before I'll poison it. As system.h is included very early, one can guarantee that there will be no usage before the poisoning happens. Martin > > > Ciao, > Michael. >
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
Hi, On Wed, 7 Nov 2018, Martin Liška wrote: > > Whereever the new function belongs it certainly isn't system.h. Also > > the definition in a header seems excessive. Sure, it enables inlining > > of it, but that seems premature optimization. It contains a loop, and > > inlining anything with loops that aren't very likely to loop just once > > or never just blows code for no gain. Also as the function is leaf > > there won't be any second-order effect from inlining. > > Ok, works for me. As you know my main motivation was to provide stronger > type declaration that can be used for 'const char *'. So what about > providing 2 wrappers and poisoning the implementation? That seems better. But still, why declare this in system.h? It seems hash-table.h seems more appropriate. Ciao, Michael.
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
On 11/5/18 7:37 PM, Michael Matz wrote:
> Hi,
>
> On Fri, 2 Nov 2018, Martin Liška wrote:
>
>> V2 of the patch.
>>
>> Thoughts?
>
Hi.
> Whereever the new function belongs it certainly isn't system.h. Also the
> definition in a header seems excessive. Sure, it enables inlining of it,
> but that seems premature optimization. It contains a loop, and inlining
> anything with loops that aren't very likely to loop just once or never
> just blows code for no gain. Also as the function is leaf there won't be
> any second-order effect from inlining.
Ok, works for me. As you know my main motivation was to provide stronger type
declaration that can be used for 'const char *'. So what about providing 2
wrappers
and poisoning the implementation?
Martin
>
>
> Ciao,
> Michael.
>
diff --git a/gcc/system.h b/gcc/system.h
index 100feb567c9..fb781645350 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -879,6 +879,23 @@ extern void fancy_abort (const char *, int, const char *)
#undef strerror
#pragma GCC poison strerror
+/* Define new hash string wappers that will allow to poison
+ the implementation. */
+
+static inline hashval_t
+hash_string_vptr (const void *p)
+{
+ return htab_hash_string (p);
+}
+
+static inline hashval_t
+hash_string (const char *str)
+{
+ return htab_hash_string ((const char *)str);
+}
+
+#pragma GCC poison htab_hash_string
+
/* loc_t is defined on some systems and too inviting for some
programmers to avoid. */
#undef loc_t
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
Hi, On Fri, 2 Nov 2018, Martin Liška wrote: > V2 of the patch. > > Thoughts? Whereever the new function belongs it certainly isn't system.h. Also the definition in a header seems excessive. Sure, it enables inlining of it, but that seems premature optimization. It contains a loop, and inlining anything with loops that aren't very likely to loop just once or never just blows code for no gain. Also as the function is leaf there won't be any second-order effect from inlining. Ciao, Michael.
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
V2 of the patch.
Thoughts?
Martin
>From 484d6d29292f210946f9d5091273eb9e1796c874 Mon Sep 17 00:00:00 2001
From: marxin
Date: Fri, 2 Nov 2018 10:15:10 +0100
Subject: [PATCH] Come up with htab_hash_string_vptr and use string-specific if
possible.
gcc/ChangeLog:
2018-11-02 Martin Liska
* system.h (hash_string): New function.
(hash_string_vptr): Likewise.
Poison htab_hash_string.
* attribs.c (struct excl_hash_traits): Use hash_string and
hash_string_vptr instead of libiberty htah_hash_string.
* config/darwin.c (indirection_hasher::hash): Likewise.
(machopic_indirection_name): Likewise.
(machopic_validate_stub_or_non_lazy_ptr): Likewise.
* config/sol2.c (comdat_entry_hasher::hash): Likewise.
* dwarf2out.c (indirect_string_hasher::hash): Likewise.
(find_AT_string_in_table): Likewise.
(addr_hasher::hash): Likewise.
(external_ref_hasher::hash): Likewise.
(dwarf_file_hasher::hash): Likewise.
(lookup_filename): Likewise.
(macinfo_entry_hasher::hash): Likewise.
(prune_unused_types_update_strings): Likewise.
* gengtype-state.c (read_state): Likewise.
* gengtype.c (htab_hash_inputfile): Likewise.
* genhooks.c (s_hook_hash): Likewise.
* genmatch.c (id_base::id_base): Likewise.
* genmodes.c (hash_mode): Likewise.
* gensupport.c (gen_mnemonic_attr): Likewise.
(check_define_attr_duplicates): Likewise.
(hash_struct_pred_data): Likewise.
* gentarget-def.c (insn_hasher::hash): Likewise.
(def_target_insn): Likewise.
(add_insn): Likewise.
* godump.c (macro_hash_hashval): Likewise.
(go_define): Likewise.
(go_finish): Likewise.
* hash-traits.h (string_hash::hash): Likewise.
* lra.c (lra_rtx_hash): Likewise.
* lto-section-in.c (hash_name): Likewise.
* plugin.c (event_hasher::hash): Likewise.
(htab_hash_plugin): Likewise.
(add_new_plugin): Likewise.
(parse_plugin_arg_opt): Likewise.
(register_plugin_info): Likewise.
(init_one_plugin): Likewise.
* read-md.c (leading_string_hash): Likewise.
* read-rtl.c (overloaded_name_hash): Likewise.
* statistics.c (stats_counter_hasher::hash): Likewise.
* symtab.c (symbol_table::decl_assembler_name_hash): Likewise.
(section_name_hasher::hash): Likewise.
(symtab_node::set_section_for_node): Likewise.
* tlink.c (hash_string_hash): Likewise.
(symbol_hash_lookup): Likewise.
(file_hash_lookup): Likewise.
(demangled_hash_lookup): Likewise.
* varasm.c (section_hasher::hash): Likewise.
(hash_section): Likewise.
(get_section): Likewise.
(const_rtx_hash_1): Likewise.
gcc/cp/ChangeLog:
2018-11-02 Martin Liska
* cp-ubsan.c (cp_ubsan_instrument_vptr): Use hash_string and
hash_string_vptr instead of libiberty htah_hash_string.
gcc/fortran/ChangeLog:
2018-11-02 Martin Liska
* trans-decl.c (struct module_hasher): Use hash_string and
hash_string_vptr instead of libiberty htah_hash_string.
(module_decl_hasher::hash): Likewise.
(gfc_find_module): Likewise.
(gfc_module_add_decl): Likewise.
(gfc_trans_use_stmts): Likewise.
gcc/lto/ChangeLog:
2018-11-02 Martin Liska
* lto.c (hash_name): Use hash_string and
hash_string_vptr instead of libiberty htah_hash_string.
libcc1/ChangeLog:
2018-11-02 Martin Liska
* libcc1plugin.cc (struct string_hasher): Use hash_string and
hash_string_vptr instead of libiberty htah_hash_string.
* libcp1plugin.cc (struct string_hasher): Likewise.
---
gcc/attribs.c| 4 ++--
gcc/config/darwin.c | 6 +++---
gcc/config/sol2.c| 2 +-
gcc/cp/cp-ubsan.c| 2 +-
gcc/dwarf2out.c | 16
gcc/fortran/trans-decl.c | 10 +-
gcc/gengtype-state.c | 2 +-
gcc/gengtype.c | 2 +-
gcc/genhooks.c | 2 +-
gcc/genmatch.c | 2 +-
gcc/genmodes.c | 2 +-
gcc/gensupport.c | 6 +++---
gcc/gentarget-def.c | 6 +++---
gcc/godump.c | 10 +-
gcc/hash-traits.h| 2 +-
gcc/lra.c| 2 +-
gcc/lto-section-in.c | 2 +-
gcc/lto/lto.c| 2 +-
gcc/plugin.c | 12 ++--
gcc/read-md.c| 2 +-
gcc/read-rtl.c | 2 +-
gcc/statistics.c | 2 +-
gcc/symtab.c | 10 +-
gcc/system.h | 24
gcc/tlink.c | 8
gcc/varasm.c | 8
libcc1/libcc1plugin.cc | 2 +-
libcc1/libcp1plugin.cc | 2 +-
28 files changed, 88 insertions(+), 64 deletions(-)
diff --git a/gcc/attribs.c b/gcc/attribs.c
index 8b721274d3b..11fe1e51b30 100644
--- a/gcc/attribs.c
+++ b/gcc/attribs.c
@@ -1828,8 +1828,8 @@ struct excl_hash_traits: typed_noop_remove
static hashval_t hash (const value_type )
{
-hashval_t h1 = htab_hash_string (x.first);
-hashval_t h2 = htab_hash_string (x.second);
+hashval_t h1 = hash_string (x.first);
+hashval_t h2 = hash_string (x.second);
return h1 ^ h2;
}
diff --git a/gcc/config/darwin.c b/gcc/config/darwin.c
index aa2ef91c64a..1c1b930c174 100644
--- a/gcc/config/darwin.c
+++
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
On 11/2/18 9:02 AM, Richard Biener wrote: > On Wed, Oct 31, 2018 at 5:40 PM Martin Liška wrote: >> >> Hi. >> >> As seen in r265663 having htab_hash_string accepting const char * would >> report a compilation error. The void * argument is needed for old C-style >> htab used in libiberty. I'm suggesting to come up with htab_hash_string_vptr >> and >> change signature of the old one (htab_hash_string). And putting these into >> hashtab.h will make it inlinable in C++-style hash table. >> >> Patch survives regression tests on ppc64le-linux-gnu. >> Total cc1 change with the patch: >> +0.0% +2.09Ki >> >> Hope it's acceptable. > > What's the reason to inline the implementation? Doing that can enable inlining of hash_table::* functions (find_slot_with_hash, ...) in gcc. I guess you want to > get a compilation error when calling htab_hash_string on a non-string? Yep. > But then libiberty isn't C++ an is used in other projects which you > may be breaking with your patch? Ah, ok, that's new for me that it's used elsewhere :) > > A solution might be to poison htab_hash_string and provide our own > variant. Will work on that. Martin > > Richard. > >> Thanks, >> Martin >> >> gcc/ChangeLog: >> >> 2018-10-31 Martin Liska >> >> * gengtype-state.c (read_state): Use newly added >> htab_hash_string_vptr. >> * gensupport.c (gen_mnemonic_attr): Likewise. >> (check_define_attr_duplicates): Likewise. >> * godump.c (go_finish): Likewise. >> >> include/ChangeLog: >> >> 2018-10-31 Martin Liska >> >> * hashtab.h (htab_hash_string): Change signature >> to const char * and make it static inline. >> (htab_hash_string_vptr): Likewise. >> >> libcpp/ChangeLog: >> >> 2018-10-31 Martin Liska >> >> * files.c (_cpp_init_files): Use htab_hash_string_vptr. >> >> libiberty/ChangeLog: >> >> 2018-10-31 Martin Liska >> >> * hashtab.c: >> (htab_hash_string): Move to header file. >> --- >> gcc/gengtype-state.c | 2 +- >> gcc/gensupport.c | 4 ++-- >> gcc/godump.c | 6 +++--- >> include/hashtab.h| 47 ++-- >> libcpp/files.c | 2 +- >> libiberty/hashtab.c | 38 --- >> 6 files changed, 52 insertions(+), 47 deletions(-) >> >>
Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.
On Wed, Oct 31, 2018 at 5:40 PM Martin Liška wrote: > > Hi. > > As seen in r265663 having htab_hash_string accepting const char * would > report a compilation error. The void * argument is needed for old C-style > htab used in libiberty. I'm suggesting to come up with htab_hash_string_vptr > and > change signature of the old one (htab_hash_string). And putting these into > hashtab.h will make it inlinable in C++-style hash table. > > Patch survives regression tests on ppc64le-linux-gnu. > Total cc1 change with the patch: > +0.0% +2.09Ki > > Hope it's acceptable. What's the reason to inline the implementation? I guess you want to get a compilation error when calling htab_hash_string on a non-string? But then libiberty isn't C++ an is used in other projects which you may be breaking with your patch? A solution might be to poison htab_hash_string and provide our own variant. Richard. > Thanks, > Martin > > gcc/ChangeLog: > > 2018-10-31 Martin Liska > > * gengtype-state.c (read_state): Use newly added > htab_hash_string_vptr. > * gensupport.c (gen_mnemonic_attr): Likewise. > (check_define_attr_duplicates): Likewise. > * godump.c (go_finish): Likewise. > > include/ChangeLog: > > 2018-10-31 Martin Liska > > * hashtab.h (htab_hash_string): Change signature > to const char * and make it static inline. > (htab_hash_string_vptr): Likewise. > > libcpp/ChangeLog: > > 2018-10-31 Martin Liska > > * files.c (_cpp_init_files): Use htab_hash_string_vptr. > > libiberty/ChangeLog: > > 2018-10-31 Martin Liska > > * hashtab.c: > (htab_hash_string): Move to header file. > --- > gcc/gengtype-state.c | 2 +- > gcc/gensupport.c | 4 ++-- > gcc/godump.c | 6 +++--- > include/hashtab.h| 47 ++-- > libcpp/files.c | 2 +- > libiberty/hashtab.c | 38 --- > 6 files changed, 52 insertions(+), 47 deletions(-) > >
