[PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
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.
Thanks,
Martin

gcc/ChangeLog:

2018-10-31  Martin Liska  <[hidden email]>

        * 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  <[hidden email]>

        * 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  <[hidden email]>

        * files.c (_cpp_init_files): Use htab_hash_string_vptr.

libiberty/ChangeLog:

2018-10-31  Martin Liska  <[hidden email]>

        * 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(-)



0001-Come-up-with-htab_hash_string_vptr-and-use-string-sp.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Richard Biener-2
On Wed, Oct 31, 2018 at 5:40 PM Martin Liška <[hidden email]> 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  <[hidden email]>
>
>         * 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  <[hidden email]>
>
>         * 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  <[hidden email]>
>
>         * files.c (_cpp_init_files): Use htab_hash_string_vptr.
>
> libiberty/ChangeLog:
>
> 2018-10-31  Martin Liska  <[hidden email]>
>
>         * 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(-)
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
On 11/2/18 9:02 AM, Richard Biener wrote:

> On Wed, Oct 31, 2018 at 5:40 PM Martin Liška <[hidden email]> 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  <[hidden email]>
>>
>>         * 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  <[hidden email]>
>>
>>         * 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  <[hidden email]>
>>
>>         * files.c (_cpp_init_files): Use htab_hash_string_vptr.
>>
>> libiberty/ChangeLog:
>>
>> 2018-10-31  Martin Liska  <[hidden email]>
>>
>>         * 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(-)
>>
>>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
V2 of the patch.

Thoughts?
Martin

0001-Come-up-with-htab_hash_string_vptr-and-use-string-sp.patch (27K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Michael Matz
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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
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.
>


hash.patch (721 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Michael Matz
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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
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.
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Michael Matz
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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Martin Liška-2
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 &p, 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  {
   215    return hash_string (id);
   216  }

So it's question whether it worth doing that?

Martin

>
>
> Ciao,
> Michael.
>

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Come up with htab_hash_string_vptr and use string-specific if possible.

Michael Matz
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.