Re: RFA: PATCH to restore old behavior of debug_vec_tree
On 7/8/13, Jason Merrill wrote: > Lawrence's overhaul of the debug() functions changed the behavior of the > pvt macro in gdbinit.in. Previously it used print_node to dump each of > the elements of the vector, but after the change it uses debug, which > both prints less information by default and fails to handle many C++ > tree nodes. > > Fixed by adding debug_raw for vec and changing > debug_vec_tree to use that. > > OK for trunk? Looks good to me. -- Lawrence Crowl
Re: [c++-concepts] code review
On 6/10/13, Diego Novillo wrote: > On 2013-06-09 20:34 , Gabriel Dos Reis wrote: > > So, my advice is for GCC source code to forget about the > > headers for the most part. I can see an instance where > > or would make a difference but given point (1) above, > > no it doesn't. Just use the traditional headers and > > be done with it. > > > > Maybe I should have included this in our C++ coding standards, > > but I don't know how Benjamin, Lawrence, and Diego fee about it. > > Sounds reasonable to me. Me too. The split in headers always felt excessively artifical to me. -- Lawrence Crowl
Re: [patch] Hash table changes from cxx-conversion branch - config part
On 5/21/13, Diego Novillo wrote: > On May 13, 2013 Lawrence Crowl wrote: > > I still have not heard from i386 or ia64 folks. Anyone? > > The i386 bits look fine to me as well. Please wait 48 hours to > give the i386 maintainers a chance to object. Committed. -- Lawrence Crowl
Re: [patch] Hash table changes from cxx-conversion branch - config part
I still have not heard from i386 or ia64 folks. Anyone?
On 4/24/13, Lawrence Crowl wrote:
> This patch is a consolodation of the hash_table patches to the
> cxx-conversion branch for files under gcc/config.
>
> Recipients:
> config/arm/arm.c - ni...@redhat.com, ramana.radhakrish...@arm.com
> config/ia64/ia64.c - wil...@tuliptree.org, sell...@mips.com
> config/mips/mips.c - rdsandif...@googlemail.com
> config/sol2.c - r...@cebitec.uni-bielefeld.de
> config/i386/winnt.c - c...@gcc.gnu.org, kti...@redhat.com
> global - rguent...@suse.de, dnovi...@google.com
>
> Update various hash tables from htab_t to hash_table.
> Modify types and calls to match.
>
> * config/arm/arm.c'arm_libcall_uses_aapcs_base::libcall_htab
>
> Fold libcall_eq and libcall_hash into new struct libcall_hasher.
>
> * config/ia64/ia64.c'bundle_state_table
>
> Fold bundle_state_hash and bundle_state_eq_p into new struct
> bundle_state_hasher.
>
> * config/mips/mips.c'mips_offset_table
>
> Fold mips_lo_sum_offset_hash and mips_lo_sum_offset_eq into new
> struct mips_lo_sum_offset_hasher.
>
> In mips_reorg_process_insns, change call to for_each_rtx to pass
> a pointer to the hash_table rather than a htab_t. This change
> requires then dereferencing that pointer in mips_record_lo_sum to
> obtain the hash_table.
>
> * config/sol2.c'solaris_comdat_htab
>
> Fold comdat_hash and comdat_eq into new struct comdat_entry_hasher.
>
> * config/i386/winnt.c'i386_pe_section_type_flags::htab
>
> * config/i386/winnt.c'i386_find_on_wrapper_list::wrappers
>
> Fold wrapper_strcmp into new struct wrapped_symbol_hasher.
>
> Tested on x86_64. Tested with config-list.mk.
>
>
> Index: gcc/ChangeLog
>
> 2013-04-24 Lawrence Crowl
>
> * config/arm/t-arm: Update for below.
>
> * config/arm/arm.c (arm_libcall_uses_aapcs_base::libcall_htab):
> Change type to hash_table. Update dependent calls and types.
>
> * config/i386/t-cygming: Update for below.
>
> * config/i386/t-interix: Update for below.
>
> * config/i386/winnt.c (i386_pe_section_type_flags::htab):
> Change type to hash_table. Update dependent calls and types.
> (i386_find_on_wrapper_list::wrappers): Likewise.
>
> * config/ia64/t-ia64: Update for below.
>
> * config/ia64/ia64.c (bundle_state_table):
> Change type to hash_table. Update dependent calls and types.
>
> * config/mips/mips.c (mips_reorg_process_insns::htab):
> Change type to hash_table. Update dependent calls and types.
>
> * config/sol2.c (solaris_comdat_htab):
> Change type to hash_table. Update dependent calls and types.
>
> * config/t-sol2: Update for above.
>
> Index: gcc/config/ia64/ia64.c
> ===
> --- gcc/config/ia64/ia64.c(revision 198213)
> +++ gcc/config/ia64/ia64.c(working copy)
> @@ -47,7 +47,7 @@ along with GCC; see the file COPYING3.
> #include "target-def.h"
> #include "common/common-target.h"
> #include "tm_p.h"
> -#include "hashtab.h"
> +#include "hash-table.h"
> #include "langhooks.h"
> #include "gimple.h"
> #include "intl.h"
> @@ -257,8 +257,6 @@ static struct bundle_state *get_free_bun
> static void free_bundle_state (struct bundle_state *);
> static void initiate_bundle_states (void);
> static void finish_bundle_states (void);
> -static unsigned bundle_state_hash (const void *);
> -static int bundle_state_eq_p (const void *, const void *);
> static int insert_bundle_state (struct bundle_state *);
> static void initiate_bundle_state_table (void);
> static void finish_bundle_state_table (void);
> @@ -8528,18 +8526,21 @@ finish_bundle_states (void)
> }
> }
>
> -/* Hash table of the bundle states. The key is dfa_state and insn_num
> - of the bundle states. */
> +/* Hashtable helpers. */
>
> -static htab_t bundle_state_table;
> +struct bundle_state_hasher : typed_noop_remove
> +{
> + typedef bundle_state value_type;
> + typedef bundle_state compare_type;
> + static inline hashval_t hash (const value_type *);
> + static inline bool equal (const value_type *, const compare_type *);
> +};
>
> /* The function returns hash of BUNDLE_STATE. */
>
> -static unsigned
> -bundle_state_hash (const void *bundle_state)
> +inline hashval_t
> +bundle_state_hasher::hash (const value_type *state)
> {
> - const struct bundle_state *const state
> -= (const struct bundle_state *) bundle_state;
>unsigned result, i;
>
>for (result = i = 0; i < dfa_stat
Re: [patch] Hash table changes from cxx-conversion branch
On 4/25/13, Diego Novillo wrote: > On 2013-04-24 15:58 , Lawrence Crowl wrote: >> * var-tracking.c'emit_note_data_def.vars >> * var-tracking.c'shared_hash_def.htab >> * var-tracking.c'changed_variables >> >> Fold variable_htab_hash, variable_htab_eq, variable_htab_free >>into new struct variable_hasher. >> Add typedef variable_table_type. >> Add typedef variable_iterator_type. > > This part is fine as well. Committed to trunk. -- Lawrence Crowl
Re: [patch] Hash table changes from cxx-conversion branch
On 4/25/13, Richard Biener wrote: > Thus, the patch is ok apart from the var-tracking.c bits which > I defer to respective maintainers. Okay, I split out the var-tracking.c changes. I've committed the rest to trunk. -- Lawrence Crowl
[patch] Hash table changes from cxx-conversion branch - config part
This patch is a consolodation of the hash_table patches to the
cxx-conversion branch for files under gcc/config.
Recipients:
config/arm/arm.c - ni...@redhat.com, ramana.radhakrish...@arm.com
config/ia64/ia64.c - wil...@tuliptree.org, sell...@mips.com
config/mips/mips.c - rdsandif...@googlemail.com
config/sol2.c - r...@cebitec.uni-bielefeld.de
config/i386/winnt.c - c...@gcc.gnu.org, kti...@redhat.com
global - rguent...@suse.de, dnovi...@google.com
Update various hash tables from htab_t to hash_table.
Modify types and calls to match.
* config/arm/arm.c'arm_libcall_uses_aapcs_base::libcall_htab
Fold libcall_eq and libcall_hash into new struct libcall_hasher.
* config/ia64/ia64.c'bundle_state_table
Fold bundle_state_hash and bundle_state_eq_p into new struct
bundle_state_hasher.
* config/mips/mips.c'mips_offset_table
Fold mips_lo_sum_offset_hash and mips_lo_sum_offset_eq into new
struct mips_lo_sum_offset_hasher.
In mips_reorg_process_insns, change call to for_each_rtx to pass
a pointer to the hash_table rather than a htab_t. This change
requires then dereferencing that pointer in mips_record_lo_sum to
obtain the hash_table.
* config/sol2.c'solaris_comdat_htab
Fold comdat_hash and comdat_eq into new struct comdat_entry_hasher.
* config/i386/winnt.c'i386_pe_section_type_flags::htab
* config/i386/winnt.c'i386_find_on_wrapper_list::wrappers
Fold wrapper_strcmp into new struct wrapped_symbol_hasher.
Tested on x86_64. Tested with config-list.mk.
Index: gcc/ChangeLog
2013-04-24 Lawrence Crowl
* config/arm/t-arm: Update for below.
* config/arm/arm.c (arm_libcall_uses_aapcs_base::libcall_htab):
Change type to hash_table. Update dependent calls and types.
* config/i386/t-cygming: Update for below.
* config/i386/t-interix: Update for below.
* config/i386/winnt.c (i386_pe_section_type_flags::htab):
Change type to hash_table. Update dependent calls and types.
(i386_find_on_wrapper_list::wrappers): Likewise.
* config/ia64/t-ia64: Update for below.
* config/ia64/ia64.c (bundle_state_table):
Change type to hash_table. Update dependent calls and types.
* config/mips/mips.c (mips_reorg_process_insns::htab):
Change type to hash_table. Update dependent calls and types.
* config/sol2.c (solaris_comdat_htab):
Change type to hash_table. Update dependent calls and types.
* config/t-sol2: Update for above.
Index: gcc/config/ia64/ia64.c
===
--- gcc/config/ia64/ia64.c (revision 198213)
+++ gcc/config/ia64/ia64.c (working copy)
@@ -47,7 +47,7 @@ along with GCC; see the file COPYING3.
#include "target-def.h"
#include "common/common-target.h"
#include "tm_p.h"
-#include "hashtab.h"
+#include "hash-table.h"
#include "langhooks.h"
#include "gimple.h"
#include "intl.h"
@@ -257,8 +257,6 @@ static struct bundle_state *get_free_bun
static void free_bundle_state (struct bundle_state *);
static void initiate_bundle_states (void);
static void finish_bundle_states (void);
-static unsigned bundle_state_hash (const void *);
-static int bundle_state_eq_p (const void *, const void *);
static int insert_bundle_state (struct bundle_state *);
static void initiate_bundle_state_table (void);
static void finish_bundle_state_table (void);
@@ -8528,18 +8526,21 @@ finish_bundle_states (void)
}
}
-/* Hash table of the bundle states. The key is dfa_state and insn_num
- of the bundle states. */
+/* Hashtable helpers. */
-static htab_t bundle_state_table;
+struct bundle_state_hasher : typed_noop_remove
+{
+ typedef bundle_state value_type;
+ typedef bundle_state compare_type;
+ static inline hashval_t hash (const value_type *);
+ static inline bool equal (const value_type *, const compare_type *);
+};
/* The function returns hash of BUNDLE_STATE. */
-static unsigned
-bundle_state_hash (const void *bundle_state)
+inline hashval_t
+bundle_state_hasher::hash (const value_type *state)
{
- const struct bundle_state *const state
-= (const struct bundle_state *) bundle_state;
unsigned result, i;
for (result = i = 0; i < dfa_state_size; i++)
@@ -8550,19 +8551,20 @@ bundle_state_hash (const void *bundle_st
/* The function returns nonzero if the bundle state keys are equal. */
-static int
-bundle_state_eq_p (const void *bundle_state_1, const void *bundle_state_2)
+inline bool
+bundle_state_hasher::equal (const value_type *state1,
+ const compare_type *state2)
{
- const struct bundle_state *const state1
-= (const struct bundle_state *) bundle_state_1;
- const struct bundle_state *const state2
-= (const struct bundle_state *) bundle_state_2;
-
return (state1->insn_num == state2->insn_num
&& memcmp (state1->dfa_st
Re: [patch] Hash table changes from cxx-conversion branch
Ping? On 3/31/13, Lawrence Crowl wrote: > On 3/28/13, Richard Biener wrote: >> On Mar 27, 2013 Lawrence Crowl wrote: >> > On 3/27/13, Richard Biener wrote: >> > > On Mar 23, 2013 Lawrence Crowl wrote: >> > > > This patch is a consolodation of the hash_table patches to >> > > > the cxx-conversion branch. >> > > > >> > > > Update various hash tables from htab_t to hash_table. >> > > > Modify types and calls to match. >> > > >> > > Ugh. Can you split it up somewhat ... like split target bits >> > > away at least? Targets may prefer to keep the old hashes for >> > > ease of branch maintainance. >> > >> > I will do that. >> > >> > > > * tree-ssa-live.c'var_map_base_init::tree_to_index >> > > > >> > > > New struct tree_int_map_hasher. >> > > >> > > I think this wants to be generalized - we have the common >> > > tree_map/tree_decl_map and tree_int_map maps in tree.h - >> > > those (and its users) should be tackled in a separate patch >> > > by providing common hashtable trails implementations. >> > >> > I will investigate for a separate patch. >> > >> > > > Remove unused: >> > > > >> > > > htab_t scop::original_pddrs >> > > > SCOP_ORIGINAL_PDDRS >> > > > >> > > > Remove unused: >> > > > >> > > > insert_loop_close_phis >> > > > insert_guard_phis >> > > > debug_ivtype_map >> > > > ivtype_map_elt_info >> > > > new_ivtype_map_elt >> > > >> > > Unused function/type removal are obvious changes. >> > > >> > > > Remove unused: >> > > > dse.c bitmap clear_alias_sets >> > > > dse.c bitmap disqualified_clear_alias_sets >> > > > dse.c alloc_pool clear_alias_mode_pool >> > > > dse.c dse_step2_spill >> > > > dse.c dse_step5_spill >> > > > graphds.h htab_t graph::indices >> > > >> > > See above. >> > >> > It wasn't obvious that the functions could be removed. :-) >> > >> > Are you saying you don't want these notations in the description? >> >> No, I was saying that removal of unused functions / types should be >> committed separately and do not need approval as they are obvious. >> If they are not obvious (I didn't look at that patch part), >> then posting separately still helps ;) > > I've split out the removals to separate patches. The remaining > work is in two independent pieces. The changes within the config > directory and the changes outside that directory. The descriptions > and patch are attached compressed due to mailer size issues. > > Okay for trunk? > > -- > Lawrence Crowl > -- Lawrence Crowl
Re: Comments on the suggestion to use infinite precision math for wide int.
On 4/8/13, Robert Dewar wrote: > On 4/8/2013 10:26 AM, Kenneth Zadeck wrote: > > On 04/08/2013 10:12 AM, Robert Dewar wrote: > > > On 4/8/2013 9:58 AM, Kenneth Zadeck wrote: > > > > yes but the relevant question for the not officially > > > > static integer constants is "in what precision are those > > > > operations to be performed in? I assume that you choose > > > > gcc types for these operations and you expect the math to > > > > be done within that type, i.e. exactly the way you expect > > > > the machine to perform. > > > > > > As I explained in an earlier message, *within* a single > > > expression, we are free to use higher precision, and we provide > > > modes that allow this up to and including the usea of infinite > > > precision. That applies not just to constant expressions but > > > to all expressions. > > > > My confusion is what you mean by "we"? Do you mean "we" the > > writer of the program, "we" the person invoking the compiler > > by the use command line options or "we", your company's > > implementation of ada? > > Sorry, bad usage, The gcc implementation of Ada allows the user > to specify by pragmas how intermediate overflow is handled. Correct me if I'm wrong, but the Ada standard doesn't require any particular maximum evaluation precision, but only that you get an exception if the values exceed the chosen maximum. > > My interpretation of your first email was that it was possible > > for the programmer to do something equivalent to adding > > attributes surrounding a block in the program to control the > > precision and overflow detection of the expressions in the block. > > And if this is so, then by the time the expression is seen by > > the middle end of gcc, those attributes will have been converted > > into tree code will evaluate the code in a well defined way by > > both the optimization passes and the target machine. > > Yes, that's a correct understanding In essence, you have moved some of the optimization from the back end to the front end. Correct? -- Lawrence Crowl
Re: Comments on the suggestion to use infinite precision math for wide int.
On 4/8/13, Richard Biener wrote:
> I advocate the infinite precision signed representation as one
> solution to avoid the issues that come up with your implementation
> (as I currently have access to) which has a representation
> with N bits of precision encoded with M <= N bits and no sign
> information. That obviously leaves operations on numbers of
> that representation with differing N undefined. You define it
> by having coded the operations which as far as I can see simply
> assume N is equal for any two operands and the effective sign for
> extending the M-bits encoding to the common N-bits precision is
> "available". A thorough specification of both the encoding scheme
> and the operation semantics is missing. I can side-step both of
> these issues nicely by simply using a infinite precision signed
> representation and requiring the client to explicitely truncate /
> extend to a specific precision when required. I also leave open
> the possibility to have the _encoding_ be always the same as an
> infinite precision signed representation but to always require
> an explicitely specified target precision for each operation
> (which rules out the use of operator overloading).
For efficiency, the machine representation of an infinite precision
number should allow for a compact one-word representation.
class infinite
{
int length;
union representation
{
int inside_word;
int *outside_words;
} field;
public:
int mod_one_word()
{
if (length == 1)
return field.inside_word;
else
return field.outside_word[0];
}
};
Also for efficiency, you want to know the modulus at the time you
do the last normal operation on it, not as a subsequent operation.
> Citing your example:
>
> 8 * 10 / 4
>
> and transforming it slightly into a commonly used pattern:
>
> (byte-size * 8 + bit-size) / 8
>
> then I argue that what people want here is this carried out in
> _infinite_ precision!
But what people want isn't really relevant, what is relevant is
what the language and/or compatiblity requires. Ideally, gcc
should accurately represent languages with both finite size and
infinite size.
> Even if byte-size happens to come from
> a sizetype TREE_INT_CST with 64bit precision. So either
> choice - having a fixed-precision representation or an
> infinite-precision representation - can and will lead to errors
> done by the programmer. And as you can easily build a
> finite precision wrapper around an infinite precision implementation
> but not the other way around it's obvious to me what the
> implementation should provide.
IIUC, the issue here is not the logical chain of implementation, but
the interface that is most helpful to the programmers in getting to
performant, correct code. I expect we need the infinite precision
forms, but also that having more concise coding for fixed-precision
would be helpful.
For mixed operations, all the languages that I know of promote
smaller operands to larger operands, so I think a reasonable
definition is possible here.
--
Lawrence Crowl
Re: Comments on the suggestion to use infinite precision math for wide int.
On 4/8/13, Kenneth Zadeck wrote:
> The other problem, which i invite you to use the full power of
> your c++ sorcery on, is the one where defining an operator so
> that wide-int + unsigned hwi is either rejected or properly
> zero extended. If you can do this, I will go along with
> your suggestion that the internal rep should be sign extended.
> Saying that constants are always sign extended seems ok, but there
> are a huge number of places where we convert unsigned hwis as
> the second operand and i do not want that to be a trap. I went
> thru a round of this, where i did not post the patch because i
> could not make this work. And the number of places where you
> want to use an hwi as the second operand dwarfs the number of
> places where you want to use a small integer constant.
You can use overloading, as in the following, which actually ignores
handling the sign in the representation.
class number {
unsigned int rep1;
int representation;
public:
number(int arg) : representation(arg) {}
number(unsigned int arg) : representation(arg) {}
friend number operator+(number, int);
friend number operator+(number, unsigned int);
friend number operator+(int, number);
friend number operator+(unsigned int, number);
};
number operator+(number n, int si) {
return n.representation + si;
}
number operator+(number n, unsigned int ui) {
return n.representation + ui;
}
number operator+(int si, number n) {
return n.representation + si;
}
number operator+(unsigned int ui, number n) {
return n.representation + ui;
}
If the argument type is of a template type parameter, then
you can test the template type via
if (std::is_signed::value)
// sign extend
else
// zero extend
See http://www.cplusplus.com/reference/type_traits/is_signed/.
If you want to handle non-builtin types that are asigne dor unsigned,
then you need to add a specialization for is_signed.
--
Lawrence Crowl
Re: [patch] Remove unused ivtype_map symbols from sese.[hc]
On 4/2/13, Richard Biener wrote: > On Mon, Apr 1, 2013 at 12:19 AM, Lawrence Crowl wrote: >> Remove unused symbols related to ivtype_map. This map does not appear to >> exist and I see no evidence of its removal in the ChangeLog. >> >> Tested on x86_64. >> >> Okay for trunk? > > Ok. Committed. -- Lawrence Crowl
[patch] Remove unused ivtype_map symbols from sese.[hc]
Remove unused symbols related to ivtype_map. This map does not appear to
exist and I see no evidence of its removal in the ChangeLog.
Tested on x86_64.
Okay for trunk?
Index: gcc/ChangeLog
2013-03-31 Lawrence Crowl
* sese.h (struct ivtype_map_elt_s): Remove unused.
(extern debug_ivtype_map): Remove unused.
(extern eq_ivtype_map_elts): Remove unused.
* sese.c (debug_ivtype_map): Removed unused.
(debug_ivtype_map_1): Removed unused.
(debug_ivtype_elt): Remove unused.
(eq_ivtype_map_elts): Remove unused.
Index: gcc/sese.c
===
--- gcc/sese.c (revision 197272)
+++ gcc/sese.c (working copy)
@@ -83,47 +83,6 @@ eq_rename_map_elts (const void *e1, cons
-/* Print to stderr the element ELT. */
-
-static void
-debug_ivtype_elt (ivtype_map_elt elt)
-{
- fprintf (stderr, "(%s, ", elt->cloog_iv);
- print_generic_expr (stderr, elt->type, 0);
- fprintf (stderr, ")\n");
-}
-
-/* Helper function for debug_ivtype_map. */
-
-static int
-debug_ivtype_map_1 (void **slot, void *s ATTRIBUTE_UNUSED)
-{
- struct ivtype_map_elt_s *entry = (struct ivtype_map_elt_s *) *slot;
- debug_ivtype_elt (entry);
- return 1;
-}
-
-/* Print to stderr all the elements of MAP. */
-
-DEBUG_FUNCTION void
-debug_ivtype_map (htab_t map)
-{
- htab_traverse (map, debug_ivtype_map_1, NULL);
-}
-
-/* Compares database elements E1 and E2. */
-
-int
-eq_ivtype_map_elts (const void *e1, const void *e2)
-{
- const struct ivtype_map_elt_s *elt1 = (const struct ivtype_map_elt_s *) e1;
- const struct ivtype_map_elt_s *elt2 = (const struct ivtype_map_elt_s *) e2;
-
- return (elt1->cloog_iv == elt2->cloog_iv);
-}
-
-
-
/* Record LOOP as occurring in REGION. */
static void
Index: gcc/sese.h
===
--- gcc/sese.h (revision 197272)
+++ gcc/sese.h (working copy)
@@ -275,17 +275,6 @@ new_rename_map_elt (tree old_name, tree
return res;
}
-/* Structure containing the mapping between the CLooG's induction
- variable and the type of the old induction variable. */
-typedef struct ivtype_map_elt_s
-{
- tree type;
- const char *cloog_iv;
-} *ivtype_map_elt;
-
-extern void debug_ivtype_map (htab_t);
-extern int eq_ivtype_map_elts (const void *, const void *);
-
/* Free and compute again all the dominators information. */
static inline void
--
Lawrence Crowl
Re: [patch] Remove unused code from dse.c.
On 3/29/13, Jeff Law wrote: > More correctly, it's been dead since an Oct 2008 patch from > Richard Henderson which introduced set_mem_attrs_for_spill which > effectively provides the alias analysis code with the information > it needs to disambiguate stack slots from everything else without > the callback. > > Lawrence, that's the critical bit of information you failed > to provide. Sorry. > Patch approved, please install. Committed. -- Lawrence Crowl
Re: [patch] Remove unused code from dse.c.
On 3/29/13, Jeff Law wrote: > On 03/29/2013 02:24 AM, Lawrence Crowl wrote: >> This patch has been in the hash-table branch for months. >> I thought it didn't quite meet the criteria for obvious, >> but it's close. >> >> >> In dse.c, remove alias hash tables that are never set. >> Remove conditions that are then never true. >> Remove functions that are then never called. >> Remove variables that are then never read. >> >> Tested on x86-64. >> >> >> Index: gcc/ChangeLog >> >> 2013-03-29 Lawrence Crowl >> >> * dse.c (clear_alias_sets): Remove never set. >> (disqualified_clear_alias_sets): Remove never set. >> (clear_alias_mode_pool): Remove never set. >> (dse_step0): Remove condition that is never true. >> (canon_address): Remove condition that is never true. >> (dse_step7): Remove condition that is never true. >> (rest_of_handle_dse): Remove condition that is never true. >> (rest_of_handle_dse::did_global): Remove never read from above. >> (dse_step2_spill): Remove never called from above. >> (dse_step5_spill): Remove never called from above. > > At what point did we stop setting clear_alias_sets? Was that > intentional or not? I do not know the answer to either question. > If that was an intentional change, then this patchset is OK as-is. > > If losing the setting of clear_alias_sets was accidental, then I > think we'll need to dig a bit further. The whole point of that > code is to prevent wild reads from "killing" the spill slot stores > being tracked. > > Of course with IRA/LRA all this may not be as important as it > once was. My view is that we have already lost the feature. The code that populates the set is gone. The remaining code has probably suffered bitrot because it is not being tested. Trying to recreate the population will probably result in inconsistencies anyway, necessitating a rewrite of the remaining code. So, the remaining code has little value, and might have negative value. -- Lawrence Crowl
[patch] Remove unused code from dse.c.
This patch has been in the hash-table branch for months.
I thought it didn't quite meet the criteria for obvious,
but it's close.
In dse.c, remove alias hash tables that are never set.
Remove conditions that are then never true.
Remove functions that are then never called.
Remove variables that are then never read.
Tested on x86-64.
Index: gcc/ChangeLog
2013-03-29 Lawrence Crowl
* dse.c (clear_alias_sets): Remove never set.
(disqualified_clear_alias_sets): Remove never set.
(clear_alias_mode_pool): Remove never set.
(dse_step0): Remove condition that is never true.
(canon_address): Remove condition that is never true.
(dse_step7): Remove condition that is never true.
(rest_of_handle_dse): Remove condition that is never true.
(rest_of_handle_dse::did_global): Remove never read from above.
(dse_step2_spill): Remove never called from above.
(dse_step5_spill): Remove never called from above.
Index: gcc/dse.c
===
--- gcc/dse.c (revision 197226)
+++ gcc/dse.c (working copy)
@@ -572,20 +572,6 @@ static alloc_pool deferred_change_pool;
static deferred_change_t deferred_change_list = NULL;
-/* This are used to hold the alias sets of spill variables. Since
- these are never aliased and there may be a lot of them, it makes
- sense to treat them specially. This bitvector is only allocated in
- calls from dse_record_singleton_alias_set which currently is only
- made during reload1. So when dse is called before reload this
- mechanism does nothing. */
-
-static bitmap clear_alias_sets = NULL;
-
-/* The set of clear_alias_sets that have been disqualified because
- there are loads or stores using a different mode than the alias set
- was registered with. */
-static bitmap disqualified_clear_alias_sets = NULL;
-
/* The group that holds all of the clear_alias_sets. */
static group_info_t clear_alias_group;
@@ -599,8 +585,6 @@ struct clear_alias_mode_holder
enum machine_mode mode;
};
-static alloc_pool clear_alias_mode_pool;
-
/* This is true except if cfun->stdarg -- i.e. we cannot do
this for vararg functions because they play games with the frame. */
static bool stores_off_frame_dead_at_return;
@@ -788,10 +772,7 @@ dse_step0 (void)
init_alias_analysis ();
- if (clear_alias_sets)
-clear_alias_group = get_group_info (NULL);
- else
-clear_alias_group = NULL;
+ clear_alias_group = NULL;
}
@@ -1189,39 +1170,6 @@ canon_address (rtx mem,
rtx expanded_address, address;
int expanded;
- /* Make sure that cselib is has initialized all of the operands of
- the address before asking it to do the subst. */
-
- if (clear_alias_sets)
-{
- /* If this is a spill, do not do any further processing. */
- alias_set_type alias_set = MEM_ALIAS_SET (mem);
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file, "found alias set %d\n", (int) alias_set);
- if (bitmap_bit_p (clear_alias_sets, alias_set))
- {
- struct clear_alias_mode_holder *entry
- = clear_alias_set_lookup (alias_set);
-
- /* If the modes do not match, we cannot process this set. */
- if (entry->mode != GET_MODE (mem))
- {
- if (dump_file && (dump_flags & TDF_DETAILS))
- fprintf (dump_file,
-"disqualifying alias set %d, (%s) != (%s)\n",
-(int) alias_set, GET_MODE_NAME (entry->mode),
-GET_MODE_NAME (GET_MODE (mem)));
-
- bitmap_set_bit (disqualified_clear_alias_sets, alias_set);
- return false;
- }
-
- *alias_set_out = alias_set;
- *group_id = clear_alias_group->id;
- return true;
- }
-}
-
*alias_set_out = 0;
cselib_lookup (mem_address, address_mode, 1, GET_MODE (mem));
@@ -2993,47 +2941,6 @@ dse_step2_nospill (void)
}
-/* Init the offset tables for the spill case. */
-
-static bool
-dse_step2_spill (void)
-{
- unsigned int j;
- group_info_t group = clear_alias_group;
- bitmap_iterator bi;
-
- /* Position 0 is unused because 0 is used in the maps to mean
- unused. */
- current_position = 1;
-
- if (dump_file && (dump_flags & TDF_DETAILS))
-{
- bitmap_print (dump_file, clear_alias_sets,
- "clear alias sets ", "\n");
- bitmap_print (dump_file, disqualified_clear_alias_sets,
- "disqualified clear alias sets ", "\n");
-}
-
- memset (group->offset_map_n, 0, sizeof(int) * group->offset_map_size_n);
- memset (group->offset_map_p, 0, sizeof(int) * group->offset_map_size_p);
- bitmap_clear (group->group_kill);
-
- /* Remove the disqualified positions from the store2_p set. */
[patch] Remove unused symbols.
Remove various unused symbols.
Tested on x86-64.
Committed as obvious.
Index: gcc/ChangeLog
* graphds.h (struct graph.indicies): Remove unused.
* graphite-poly.h (struct graph.original_pddrs): Remove unused.
(SCOP_ORIGINAL_PDDRS): Remove unused.
* sese.h (extern insert_loop_close_phis): Removed unused.
(extern insert_guard_phis): Removed unused.
(extern ivtype_map_elt_info): Removed unused.
(new_ivtype_map_elt): Removed unused.
* sese.c (ivtype_map_elt_info): Removed unused.
2013-03-28 Lawrence Crowl
Index: gcc/sese.c
===
--- gcc/sese.c (revision 197224)
+++ gcc/sese.c (working copy)
@@ -111,14 +111,6 @@ debug_ivtype_map (htab_t map)
htab_traverse (map, debug_ivtype_map_1, NULL);
}
-/* Computes a hash function for database element ELT. */
-
-hashval_t
-ivtype_map_elt_info (const void *elt)
-{
- return htab_hash_pointer (((const struct ivtype_map_elt_s *) elt)->cloog_iv);
-}
-
/* Compares database elements E1 and E2. */
int
Index: gcc/sese.h
===
--- gcc/sese.h (revision 197224)
+++ gcc/sese.h (working copy)
@@ -58,8 +58,6 @@ extern void build_sese_loop_nests (sese)
extern edge copy_bb_and_scalar_dependences (basic_block, sese, edge,
vec , bool *);
extern struct loop *outermost_loop_in_sese (sese, basic_block);
-extern void insert_loop_close_phis (htab_t, loop_p);
-extern void insert_guard_phis (basic_block, edge, edge, htab_t, htab_t);
extern tree scalar_evolution_in_region (sese, loop_p, tree);
/* Check that SESE contains LOOP. */
@@ -286,23 +284,8 @@ typedef struct ivtype_map_elt_s
} *ivtype_map_elt;
extern void debug_ivtype_map (htab_t);
-extern hashval_t ivtype_map_elt_info (const void *);
extern int eq_ivtype_map_elts (const void *, const void *);
-/* Constructs a new SCEV_INFO_STR structure for VAR and INSTANTIATED_BELOW. */
-
-static inline ivtype_map_elt
-new_ivtype_map_elt (const char *cloog_iv, tree type)
-{
- ivtype_map_elt res;
-
- res = XNEW (struct ivtype_map_elt_s);
- res->cloog_iv = cloog_iv;
- res->type = type;
-
- return res;
-}
-
/* Free and compute again all the dominators information. */
static inline void
Index: gcc/graphds.h
===
--- gcc/graphds.h (revision 197224)
+++ gcc/graphds.h (working copy)
@@ -46,7 +46,6 @@ struct graph
int n_vertices; /* Number of vertices. */
struct vertex *vertices;
/* The vertices. */
- htab_t indices; /* Fast lookup for indices. */
};
struct graph *new_graph (int);
Index: gcc/graphite-poly.h
===
--- gcc/graphite-poly.h (revision 197224)
+++ gcc/graphite-poly.h (working copy)
@@ -1376,10 +1376,6 @@ struct scop
*must_war, *may_war, *must_war_no_source, *may_war_no_source,
*must_waw, *may_waw, *must_waw_no_source, *may_waw_no_source;
- /* A hashtable of the data dependence relations for the original
- scattering. */
- htab_t original_pddrs;
-
/* True when the scop has been converted to its polyhedral
representation. */
bool poly_scop_p;
@@ -1388,7 +1384,6 @@ struct scop
#define SCOP_BBS(S) (S->bbs)
#define SCOP_REGION(S) ((sese) S->region)
#define SCOP_CONTEXT(S) (NULL)
-#define SCOP_ORIGINAL_PDDRS(S) (S->original_pddrs)
#define SCOP_ORIGINAL_SCHEDULE(S) (S->original_schedule)
#define SCOP_TRANSFORMED_SCHEDULE(S) (S->transformed_schedule)
#define SCOP_SAVED_SCHEDULE(S) (S->saved_schedule)
--
Lawrence Crowl
Re: [patch] Unified debug dump function names.
On 3/28/13, Richard Biener wrote: > The patch is ok as-is. Committed. -- Lawrence Crowl
Re: [patch] Hash table changes from cxx-conversion branch
On 3/27/13, Richard Biener wrote: > On Mar 23, 2013 Lawrence Crowl wrote: > > This patch is a consolodation of the hash_table patches to the > > cxx-conversion branch. > > > > Update various hash tables from htab_t to hash_table. > > Modify types and calls to match. > > Ugh. Can you split it up somewhat ... like split target bits > away at least? Targets may prefer to keep the old hashes for > ease of branch maintainance. I will do that. > > * tree-ssa-live.c'var_map_base_init::tree_to_index > > > > New struct tree_int_map_hasher. > > I think this wants to be generalized - we have the common > tree_map/tree_decl_map and tree_int_map maps in tree.h - those > (and its users) should be tackled in a separate patch by providing > common hashtable trails implementations. I will investigate for a separate patch. > > Remove unused: > > > > htab_t scop::original_pddrs > > SCOP_ORIGINAL_PDDRS > > > > Remove unused: > > > > insert_loop_close_phis > > insert_guard_phis > > debug_ivtype_map > > ivtype_map_elt_info > > new_ivtype_map_elt > > Unused function/type removal are obvious changes. > > > Remove unused: > > dse.c bitmap clear_alias_sets > > dse.c bitmap disqualified_clear_alias_sets > > dse.c alloc_pool clear_alias_mode_pool > > dse.c dse_step2_spill > > dse.c dse_step5_spill > > graphds.h htab_t graph::indices > > See above. It wasn't obvious that the functions could be removed. :-) Are you saying you don't want these notations in the description? -- Lawrence Crowl
Re: [patch] Unified debug dump function names.
On 3/27/13, Richard Biener wrote:
> On Mar 27, 2013, Lawrence Crowl wrote:
>> Patch with rename to debug attached.
>> Tested on x86_64.
>>
>>
>> Add uniform debug dump function names.
>>
>>
>> Add some overloaded functions that provide uniform debug dump
>> function names. These names are:
>>
>> debug: the general debug dumper
>> debug_verbose: for more details
>> debug_raw: for the gory details
>> debug_head: for the heads of declarations, e.g. function heads
>> debug_body: for the bodies of declarations, e.g. function bodies
>>
>> Not all types have the last four versions.
>>
>> The debug functions come in two flavors, those that take pointers
>> to the type, and those that take references to the type. The first
>> handles printing of '' for null pointers. The second assumes
>> a valid reference, and prints the content.
>>
>>
>> Example uses are as follows:
>>
>> cp_token t, *p;
>> debug (t);
>> debug (p);
>>
>> From the debugger, use
>>
>> call debug (t)
>>
>>
>> The functions sets implemented are:
>>
>> debug (only)
>>
>> basic_block_def, const bitmap_head_def, cp_binding_level,
>> cp_parser, cp_token, data_reference, die_struct, edge_def,
>> gimple_statement_d, ira_allocno, ira_allocno_copy, live_range,
>> lra_live_range, omega_pb_d, pt_solution, const rtx_def, sreal,
>> tree_live_info_d, _var_map,
>>
>> vec, vec, vec,
>> vec, vec,
>>
>> debug and debug_raw
>>
>> simple_bitmap_def
>>
>> debug and debug_verbose
>>
>> expr_def, struct loop, vinsn_def
>>
>> debug, debug_raw, debug_verbose, debug_head, debug_body
>>
>> const tree_node
>>
>>
>> This patch is somewhat different from the original plan at
>> gcc.gnu.org/wiki/cxx-conversion/debugging-dumps. The reason
>> is that gdb has an incomplete implementation of C++ call syntax;
>> requiring explicit specification of template arguments and explicit
>> specification of function arguments even when they have default
>> values. So, the original plan would have required typing
>>
>> call dump (t, 0, 0, stderr)
>>
>> which is undesireable. Instead instead of templates, we overload
>> plain functions. This adds a small burden of manually adding
>> the pointer version of dump for each type. Instead of default
>> function arguments, we simply assume the default values. Most of
>> the underlying dump functions did not use the options and indent
>> parameters anyway. Several provide FILE* parameters, but we expect
>> debugging to use stderr anyway. So, the explicit specification of
>> arguments was not as valuable as we thought initially.
>
> Note that generally modules should provide a
>
> print_FOO (FILE *, FOO *...)
>
> interface which should be the worker for the dump_* interface
> which prints to dumpfiles (and stdout/stderr with -fopt-info) and
> the debug_* interface (which just prints to stderr).
I'm not sure what you're saying here. I haven't been adding new
underlying functionality. If there are missing print_FOO functions,
shouldn't they be a separate work item?
>> Finally, a change of name from dump to debug reflect the implicit
>> output to stderr.
>
> A few more questions. As we are now using C++ and these
> functions are not called by GCC itself - do we really need
> all the extern declarations in the header files? We don't have
> -Wstrict-prototypes issues with C++ and I do not consider the debug
> () interface part of the public API of a module. This avoids
> people ending up calling debug () from inside GCC.
We don't need the extern declarations for gdb, but I've written
temporary calls to debug into the source code while I was developing.
It would be handy to not have to add declarations simultaneously.
Your call.
> + if (ptr)
> +debug (*ptr);
> + else
> +fprintf (stderr, "\n");
>
> can we avoid repeating this using a common helper? I'd use a simple
> #define like
>
> #define DO_DEBUG_PTR(p) do { if (p) debug (*(p)) else fprintf (stderr,
> "\n"); } while (0)
>
> but I suppose you can come up with something more clever using C++?
I had a template that did this for us in my earlier code. I removed
the template when I discovered the gdb issue. One advantage to
removing the template was that I no longer needed a common header and
its inclusion in various files. Adding the macro would reintroduce
the header.
My personal preference is to avoid the macros and just live with the
repeated pattern.
--
Lawrence Crowl
Re: [patch] Unified debug dump function names.
On 3/26/13, Lawrence Crowl wrote: > On 3/26/13, Richard Biener wrote: >> On Mar 25, 2013 Lawrence Crowl wrote: >> > On 3/25/13, Richard Biener wrote: >> > > You add a not used new interface. What for? >> > >> > So that people can use it. >> > >> > > For use from gdb only? >> > >> > No, for use from both gdb and internally. It is often that >> > folks add dumps in various places while developing/debugging. >> > These functions support that effort without having to hunt down >> > the name. >> >> But having both interfaces is bad. As you are unconditionally >> "dumping" to stderr using debug () is correct. Sorry that I >> don't follow each and every proposal - nobody follows up my >> proposals either. >> >> The dump_ namespace is claimed by dumping to dumpfiles and >> diagnostics. >> >> > > In which case it should be debug (), not dump (). >> > >> > I will use whatever name you wish, but I would have preferred >> > that we addressed naming issues when we published the plan, >> > not after I've done the implementation. What name do you wish? >> >> debug (). > > Okay. > >> And I'd like you to remove the alternate debug_ interface that >> is obsoleted by the overloads. > > I'm sure that folks have the old interfaces baked into scripts and > dot files. I think it would should not remove the old interface > until they have had some time to migrate. > >> Btw, the overloading will provide extra headache to one of the >> most used ways to the debug_ routines: >> >> (gdb) call debug_tree (fndecl) >> >type >type >size >>unit size >>align 32 symtab 0 alias set -1 canonical type >> 0x76d175e8 precision 32 min > -2147483648> max >> ... >> (gdb) call debug_tree (0x76d175e8) >> Cannot resolve function debug_tree to any overloaded instance >> (gdb) call debug_tree >> debug_tree(tree_node*) >> debug_tree_chain(tree_node*) >> debug_tree_chain(tree_node*)::__FUNCTION__ >> debug_tree_ssa() >> debug_tree_ssa_stats() >> >> (gdb) call debug_tree ((tree_node*)0x76d175e8) >> >size > bitsizetype> constant 32> >>unit size > 0x76d17000 sizetype> constant 4> >>align 32 symtab 0 alias set -1 canonical type 0x76d175e8 >> precision 32 min max >> >>pointer_to_this > >> >> but with debug () having overloads to each and every thing we'd >> ever want to debug the list of possible types I have to cast that >> literal address I cut&pasted will be endless. >> >> Any suggestion on how to improve this situation? Yes, it's already >> bad as with typing debug_tree I know it's a tree I am interested >> in and >> >> (gdb) call debug_ >> ... endless list of functions and overloads ... >> >> is probably as useless as >> >> (gdb) call debug >> >> is after your patch. > > I have three suggestions, in increasing order of difficulty. > > First, modify the dumpers to print the type cast in front of > the hex value. The cut and paste is just a bit wider. > > Second, modify the dumpers to print the access expression (which > would then not require the hex value). I'm not actually sure how > well this would work in practice. It's a thought. > > Third, modify gdb to have an interactive data explorer. As a > straw man, "explorer foo" would open up a window with all of > foo's elements. Each pointer is clickable and changes your view to > its referrent. I've used such a tool, and while it was sometimes > at too low a level of abstraction, it was generally quite handy > for exploring the data. In retrospect, it would be nice to fold > sub-objects (in the editor sense). Patch with rename to debug attached. Tested on x86_64. Add uniform debug dump function names. Add some overloaded functions that provide uniform debug dump function names. These names are: debug: the general debug dumper debug_verbose: for more details debug_raw: for the gory details debug_head: for the heads of declarations, e.g. function heads debug_body: for the bodies of declarations, e.g. function bodies Not all types have the last four versions. The debug functions come in two flavors, those that take pointers to the type, and those that take references to the type. The first handles printing of '' for null pointers. The second assumes a valid reference, and prints the content.
Re: [patch] Unified debug dump function names.
On 3/26/13, Richard Biener wrote: > On Mar 25, 2013 Lawrence Crowl wrote: > > On 3/25/13, Richard Biener wrote: > > > You add a not used new interface. What for? > > > > So that people can use it. > > > > > For use from gdb only? > > > > No, for use from both gdb and internally. It is often that > > folks add dumps in various places while developing/debugging. > > These functions support that effort without having to hunt down > > the name. > > But having both interfaces is bad. As you are unconditionally > "dumping" to stderr using debug () is correct. Sorry that I > don't follow each and every proposal - nobody follows up my > proposals either. > > The dump_ namespace is claimed by dumping to dumpfiles and > diagnostics. > > > > In which case it should be debug (), not dump (). > > > > I will use whatever name you wish, but I would have preferred > > that we addressed naming issues when we published the plan, > > not after I've done the implementation. What name do you wish? > > debug (). Okay. > And I'd like you to remove the alternate debug_ interface that > is obsoleted by the overloads. I'm sure that folks have the old interfaces baked into scripts and dot files. I think it would should not remove the old interface until they have had some time to migrate. > Btw, the overloading will provide extra headache to one of the > most used ways to the debug_ routines: > > (gdb) call debug_tree (fndecl) > type type size >unit size >align 32 symtab 0 alias set -1 canonical type > 0x76d175e8 precision 32 min -2147483648> max > ... > (gdb) call debug_tree (0x76d175e8) > Cannot resolve function debug_tree to any overloaded instance > (gdb) call debug_tree > debug_tree(tree_node*) > debug_tree_chain(tree_node*) > debug_tree_chain(tree_node*)::__FUNCTION__ > debug_tree_ssa() > debug_tree_ssa_stats() > > (gdb) call debug_tree ((tree_node*)0x76d175e8) > size bitsizetype> constant 32> >unit size 0x76d17000 sizetype> constant 4> >align 32 symtab 0 alias set -1 canonical type 0x76d175e8 > precision 32 min max > >pointer_to_this > > > but with debug () having overloads to each and every thing we'd > ever want to debug the list of possible types I have to cast that > literal address I cut&pasted will be endless. > > Any suggestion on how to improve this situation? Yes, it's already > bad as with typing debug_tree I know it's a tree I am interested > in and > > (gdb) call debug_ > ... endless list of functions and overloads ... > > is probably as useless as > > (gdb) call debug > > is after your patch. I have three suggestions, in increasing order of difficulty. First, modify the dumpers to print the type cast in front of the hex value. The cut and paste is just a bit wider. Second, modify the dumpers to print the access expression (which would then not require the hex value). I'm not actually sure how well this would work in practice. It's a thought. Third, modify gdb to have an interactive data explorer. As a straw man, "explorer foo" would open up a window with all of foo's elements. Each pointer is clickable and changes your view to its referrent. I've used such a tool, and while it was sometimes at too low a level of abstraction, it was generally quite handy for exploring the data. In retrospect, it would be nice to fold sub-objects (in the editor sense). -- Lawrence Crowl
Re: [patch] Unified debug dump function names.
On 3/25/13, Tom Tromey wrote: >>>>>> "Lawrence" == Lawrence Crowl writes: > Lawrence> This patch is somewhat different from the original plan at > Lawrence> gcc.gnu.org/wiki/cxx-conversion/debugging-dumps. The reason > Lawrence> is that gdb has an incomplete implementation of C++ call syntax; > Lawrence> requiring explicit specification of template arguments and > Lawrence> explicit specification of function arguments even when they have > Lawrence> default values. > > Note that the latter is because GCC doesn't emit this information. I'm not laying blame anywhere, just informing folks of an adjustment to the plan due to the current situation. > As for the former ... we have a patch that works in some cases, > but it's actually unclear to me how well the debugger can do > in general here. We haven't put it in since it seems better to > require users to be explicit than to silently do the wrong thing > in some cases. My model is that I should be able to cut and paste an expression from the source to the debugger and have it work. I concede that C++ function overload resolution is a hard problem. However, gdb has a slightly easier task in that it won't be doing instantiation (as that expression has already instantiated everything it needs) and so it need only pick among what exists. -- Lawrence Crowl
Re: [patch] Unified debug dump function names.
On 3/25/13, Richard Biener wrote: > You add a not used new interface. What for? So that people can use it. > For use from gdb only? No, for use from both gdb and internally. It is often that folks add dumps in various places while developing/debugging. These functions support that effort without having to hunt down the name. > In which case it should be debug (), not dump (). I will use whatever name you wish, but I would have preferred that we addressed naming issues when we published the plan, not after I've done the implementation. What name do you wish? -- Lawrence Crowl
Re: [cxx-conversion] RFC - Helper types for building GIMPLE
YPE (op);
> +}
> +
> +
> +/* Add a new assignment to this GIMPLE sequence. The assignment has
> + the form: GIMPLE_ASSIGN . Returns LHS. */
> +
> +tree
> +gimple_builder_base::add (enum tree_code code, tree lhs, tree op1, tree
> op2)
> +{
> + gimple s = gimple_build_assign_with_ops (code, lhs, op1, op2);
> + gimple_seq_add_stmt (&seq_, s);
> + return lhs;
> +}
> +
> +
> +/* Add a new assignment to this GIMPLE sequence. The new assignment will
> + have the opcode CODE and operands OP1 and OP2. The type of the
> + expression on the RHS is inferred to be the type of OP1.
> +
> + The LHS of the statement will be an SSA name or a GIMPLE temporary
> + in normal form depending on the type of builder invoking this
> + function. */
> +
> +tree
> +gimple_builder_base::add (enum tree_code code, tree op1, tree op2)
> +{
> + tree lhs = create_lhs_for_assignment (get_expr_type (code, op1));
> + return add (code, lhs, op1, op2);
> +}
> +
> +
> +/* Add a new assignment to this GIMPLE sequence. The new
> + assignment will have the opcode CODE and operands OP1 and VAL.
> + VAL is converted into a an INTEGER_CST of the same type as OP1.
> +
> + The LHS of the statement will be an SSA name or a GIMPLE temporary
> + in normal form depending on the type of builder invoking this
> + function. */
> +
> +tree
> +gimple_builder_base::add (enum tree_code code, tree op1, int val)
> +{
> + tree type = get_expr_type (code, op1);
> + tree op2 = build_int_cst (TREE_TYPE (op1), val);
> + tree lhs = create_lhs_for_assignment (type);
> + return add (code, lhs, op1, op2);
> +}
> +
> +
> +/* Add a type cast assignment to this GIMPLE sequence. This creates a
> NOP_EXPR
> + that converts OP to TO_TYPE. Return the LHS of the generated
> assignment. */
> +
> +tree
> +gimple_builder_base::add_type_cast (tree to_type, tree op)
> +{
> + tree lhs = create_lhs_for_assignment (to_type);
> + return add (NOP_EXPR, lhs, op, NULL_TREE);
> +}
> +
> +
> +/* Insert this sequence after the statement pointed-to by iterator I.
> + MODE is an is gs_insert_after. Scan the statements in SEQ for new
> + operands. */
> +
> +void
> +gimple_builder_base::insert_after (gimple_stmt_iterator *i,
> +enum gsi_iterator_update mode)
> +{
> + /* Since we are inserting a sequence, the semantics for GSI_NEW_STMT
> + are not quite what the caller is expecting. GSI_NEW_STMT will
> + leave the iterator pointing to the *first* statement of this
> + sequence. Rather, we want the iterator to point to the *last*
> + statement in the sequence. Therefore, we use
> + GSI_CONTINUE_LINKING when GSI_NEW_STMT is requested. */
> + if (mode == GSI_NEW_STMT)
> +mode = GSI_CONTINUE_LINKING;
> + gsi_insert_seq_after (i, seq_, mode);
> +}
> +
> +
> +/* Create a GIMPLE temporary type TYPE to be used as the LHS of an
> + assignment. */
> +
> +tree
> +gimple_builder_normal::create_lhs_for_assignment (tree type)
> +{
> + return create_tmp_var (type, NULL);
> +}
> +
> +
> +/* Create an SSA name of type TYPE to be used as the LHS of an assignment.
> */
> +
> +tree
> +gimple_builder_ssa::create_lhs_for_assignment (tree type)
> +{
> + return make_ssa_name (type, NULL);
> +}
> +
> #include "gt-gimple.h"
> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index 204c3c9..7b5e741 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -5393,4 +5393,57 @@ extern tree maybe_fold_or_comparisons (enum
> tree_code, tree, tree,
> enum tree_code, tree, tree);
>
> bool gimple_val_nonnegative_real_p (tree);
> +
> +
> +/* GIMPLE builder class. This type provides a simplified interface
> + for generating new GIMPLE statements. */
> +
> +class gimple_builder_base
> +{
> +public:
> + tree add (enum tree_code, tree, tree);
> + tree add (enum tree_code, tree, int);
> + tree add (enum tree_code, tree, tree, tree);
> + tree add_type_cast (tree, tree);
> + void insert_after (gimple_stmt_iterator *, enum gsi_iterator_update);
> +
> +protected:
> + gimple_builder_base() : seq_(NULL), loc_(UNKNOWN_LOCATION) {}
> + gimple_builder_base(location_t l) : seq_(NULL), loc_(l) {}
> + tree get_expr_type (enum tree_code code, tree op);
> + virtual tree create_lhs_for_assignment (tree) = 0;
> +
> +private:
> + gimple_seq seq_;
> + location_t loc_;
> +};
> +
> +
> +/* GIMPLE builder class for statements in normal form. Statements
> generated
> + by instances of this class will produce non-SSA temporaries. */
> +
> +class gimple_builder_normal : public gimple_builder_base
> +{
> +public:
> + gimple_builder_normal() : gimple_builder_base() {}
> + gimple_builder_normal(location_t l) : gimple_builder_base(l) {}
> +
> +protected:
> + virtual tree create_lhs_for_assignment (tree);
> +};
> +
> +
> +/* GIMPLE builder class for statements in normal form. Statements
> generated
> + by instances of this class will produce SSA names. */
> +
> +class gimple_builder_ssa : public gimple_builder_base
> +{
> +public:
> + gimple_builder_ssa() : gimple_builder_base() {}
> + gimple_builder_ssa(location_t l) : gimple_builder_base(l) {}
> +
> +protected:
> + virtual tree create_lhs_for_assignment (tree);
> +};
> +
> #endif /* GCC_GIMPLE_H */
>
--
Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
On 2/14/13, Richard Biener wrote:
> On Tue, Feb 12, 2013 at 8:47 PM, Lawrence Crowl wrote:
> > Add class record_builder to ease construction of records and unions. Use
> > it
> > in some appropriate places.
> > tree
> > -default_emutls_var_fields (tree type, tree *name ATTRIBUTE_UNUSED)
> > +default_emutls_object_type (void)
> > {
> > - tree word_type_node, field, next_field;
> > -
> > - field = build_decl (UNKNOWN_LOCATION,
> > - FIELD_DECL, get_identifier ("__templ"),
> > ptr_type_node);
> > - DECL_CONTEXT (field) = type;
> > - next_field = field;
> > -
> > - field = build_decl (UNKNOWN_LOCATION,
> > - FIELD_DECL, get_identifier ("__offset"),
> > - ptr_type_node);
> > - DECL_CONTEXT (field) = type;
> > - DECL_CHAIN (field) = next_field;
> > - next_field = field;
> > -
> > - word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> > - field = build_decl (UNKNOWN_LOCATION,
> > - FIELD_DECL, get_identifier ("__align"),
> > - word_type_node);
> > - DECL_CONTEXT (field) = type;
> > - DECL_CHAIN (field) = next_field;
> > - next_field = field;
> > -
> > - field = build_decl (UNKNOWN_LOCATION,
> > - FIELD_DECL, get_identifier ("__size"),
> > word_type_node);
> > - DECL_CONTEXT (field) = type;
> > - DECL_CHAIN (field) = next_field;
> > -
> > - return field;
> > + tree word_type_node = lang_hooks.types.type_for_mode (word_mode, 1);
> > + record_builder rec;
> > + rec.add_field ("__size", word_type_node);
> > + rec.add_field ("__align", word_type_node);
> > + rec.add_field ("__offset", ptr_type_node);
> > + rec.add_field ("__templ", ptr_type_node);
> > + rec.layout ();
>
> That's awkward - you want to hide the fact that layout has
> to happen and that it has to happen "last". Just make it a
> side-effect of .as_tree ().
Sometimes you want to construct recursive types, and for that you
need .as_tree to execute before layout. This feature is used in
the patch.
> Note that add_field want's to return the FIELD_DECL created,
> people may want to alter it.
Do you have a use case? Until we have one, I'm not convinced that
we should widen the interface.
> Note that tag_name does not allow the way C++ uses this (it can
> be a TYPE_DECL).
That is what the .decl_name member function does.
> Overall I'm not sure this is a good abstraction unless you manage
> to make the frontends use it.
The intent is for use by the middle/back ends constructing code.
As such, it should be using middle/back end types, not front-end
types.
--
Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
On 2/13/13, Diego Novillo wrote: > Thanks. The patch is OK for the branch. You can address Nathan's > review after he's back and gets a chance to look at it. > > Let me know when the patch is in. I've got another merge in process. Committed. -- Lawrence Crowl
Re: [cxx-conversion] Add Record Builder Class
On 2/13/13, Diego Novillo wrote:
> On Tue, Feb 12, 2013 at 2:47 PM, Lawrence Crowl wrote:
>> @@ -182,24 +163,9 @@ default_emutls_var_init (tree to, tree d
>> static tree
>> get_emutls_object_type (void)
>> {
>> - tree type, type_name, field;
>> -
>> - type = emutls_object_type;
>> - if (type)
>> -return type;
>> -
>> - emutls_object_type = type = lang_hooks.types.make_type (RECORD_TYPE);
>> - type_name = NULL;
>> - field = targetm.emutls.var_fields (type, &type_name);
>> - if (!type_name)
>> -type_name = get_identifier ("__emutls_object");
>> - type_name = build_decl (UNKNOWN_LOCATION,
>> - TYPE_DECL, type_name, type);
>> - TYPE_NAME (type) = type_name;
>> - TYPE_FIELDS (type) = field;
>> - layout_type (type);
>> -
>> - return type;
>> + if (!emutls_object_type)
>> +emutls_object_type = targetm.emutls.object_type ();
>> + return emutls_object_type;
>
> Hm, this does not look like an idempotent change. Where did I
> get lost?
The responsibility for creating the type has moved into the new hook.
The old hook only had responsibility for adding fields. And changing
the name if it wasn't right. The new structure has a much clearer
division of responsibility.
> ===
>> --- gcc/coverage.c (revision 195904)
>> +++ gcc/coverage.c (working copy)
>> @@ -121,8 +121,8 @@ static const char *const ctr_names[GCOV_
>> /* Forward declarations. */
>> static void read_counts_file (void);
>> static tree build_var (tree, tree, int);
>> -static void build_fn_info_type (tree, unsigned, tree);
>> -static void build_info_type (tree, tree);
>> +static tree build_fn_info_type (unsigned, tree);
>> +static tree build_info_type (record_builder &, tree);
>
> I don't really like unnecessary forward declarations. But I guess
> it's OK, since you are replacing existing ones.
Yeah. I figure applying style changes should be a separate patch.
>> -static void
>> -build_fn_info_type (tree type, unsigned counters, tree gcov_info_type)
>> +static tree
>> +build_fn_info_type (unsigned counters, tree gcov_info_type)
>
> So, here you are changing the signature because the caller used
> to create an empty record and now it doesn't need to? We are
> going to be creating it in the constructor, right?
Correct.
--
Lawrence Crowl
[cxx-conversion] Add Record Builder Class
source_location loc = UNKNOWN_LOCATION); + void add_field (const char *ident, tree type, + source_location loc = UNKNOWN_LOCATION); + void layout (); + void layout (tree align_type); + void tag_name (tree ident); + void tag_name (const char *ident); + void decl_name (tree ident, source_location loc = UNKNOWN_LOCATION); + void decl_name (const char *ident, source_location loc = UNKNOWN_LOCATION); + tree as_tree (); +private: + tree building_; + tree last_field_; +}; // class record_builder + + #endif /* GCC_TREE_H */ -- Lawrence Crowl
Re: [PATCH 1/2] [asan] Allow creating/deleting hash table entries with new/delete
On 1/28/13, Dodji Seketeli wrote:
> Hello,
>
> The hash table type can handle creation and removal of entries with
> malloc/free. This patchlet adds support for using new/delete. It's
> useful for hash table entry types that have constructors (and/or
> destructors), to prevent the user from having to type boilerplate code
> to initialize them over and over again. This is used by the patch that
> follows this one.
Looks good to me.
>
> gcc/
>
> * hash-table.h (struct typed_delete_remove): New type.
> (typed_delete_remove::remove): Implement this using the delete
> operator.
> ---
> gcc/hash-table.h | 16
> 1 file changed, 16 insertions(+)
>
> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
> index 206423d..884840c 100644
> --- a/gcc/hash-table.h
> +++ b/gcc/hash-table.h
> @@ -235,6 +235,22 @@ typed_free_remove ::remove (Type *p)
>free (p);
> }
>
> +/* Helpful type for removing entries with the delete operator. */
> +
> +template
> +struct typed_delete_remove
> +{
> + static inline void remove (Type *p);
> +};
> +
> +/* Remove with delete. */
> +
> +template
> +inline void
> +typed_delete_remove ::remove (Type *p)
> +{
> + delete p;
> +}
>
> /* Helpful type for a no-op remove. */
>
> --
> 1.7.11.7
>
>
>
> --
> Dodji
>
--
Lawrence Crowl
Re: [cxx-conversion] Change uses of htab_t in gcc/config to hash_table.
On 1/6/13, Richard Biener wrote:
> On Sun, Jan 6, 2013 at 6:55 AM, Xinliang David Li wrote:
>> I noticed that the traverse and traverse_noresize method takes
>> Argument as the first template parameter. It should be moved to be the
>> second after the callback. In most of the cases, the type of the
>> Argument can be deduced from the callsite, so that the user only need
>> to specify the callback:
>>
>> ht->traverse_noresize(&arg);
>>
>> In the current way, user has to do this:
>>
>> ht->traverse_noresize (&arg);
>>
>> which is not as friendly.
>
> Agreed.
Agreed. The current structure was handy in the conversion process.
Now that the conversion is done, we could change the order.
>> David
>>
>>
>> On Tue, Dec 18, 2012 at 8:02 PM, Lawrence Crowl
>> wrote:
>>> Update various config htab_t uses to hash_table.
>>>
>>> Modify types and calls to match.
>>>
>>> * config/arm/arm.c'arm_libcall_uses_aapcs_base::libcall_htab
>>>
>>> Fold libcall_eq and libcall_hash into new struct libcall_hasher.
>>>
>>> * config/ia64/ia64.c'bundle_state_table
>>>
>>> Fold bundle_state_hash and bundle_state_eq_p into new struct
>>> bundle_state_hasher.
>>>
>>> * config/mips/mips.c'mips_offset_table
>>>
>>> Fold mips_lo_sum_offset_hash and mips_lo_sum_offset_eq into new
>>> struct mips_lo_sum_offset_hasher.
>>>
>>> In mips_reorg_process_insns, change call to for_each_rtx to pass
>>> a pointer to the hash_table rather than a htab_t. This change
>>> requires then dereferencing that pointer in mips_record_lo_sum to
>>> obtain the hash_table.
>>>
>>> * config/sol2.c'solaris_comdat_htab
>>>
>>> Fold comdat_hash and comdat_eq into new struct comdat_entry_hasher.
>>>
>>> * config/i386/winnt.c'i386_pe_section_type_flags::htab
>>>
>>> * config/i386/winnt.c'i386_find_on_wrapper_list::wrappers
>>>
>>> Fold wrapper_strcmp into new struct wrapped_symbol_hasher.
>>>
>>>
>>> Tested on x86-64.
>>> Tested with contrib/config-list.mk.
>>>
>>>
>>> Okay for branch?
>>>
>>>
>>> Index: gcc/config/arm/arm.c
>>> ===
>>> --- gcc/config/arm/arm.c(revision 194511)
>>> +++ gcc/config/arm/arm.c(working copy)
>>> @@ -25,6 +25,7 @@
>>> #include "config.h"
>>> #include "system.h"
>>> #include "coretypes.h"
>>> +#include "hash-table.h"
>>> #include "tm.h"
>>> #include "rtl.h"
>>> #include "tree.h"
>>> @@ -3716,36 +3717,48 @@ arm_function_value(const_tree type, cons
>>>return arm_libcall_value_1 (mode);
>>> }
>>>
>>> -static int
>>> -libcall_eq (const void *p1, const void *p2)
>>> +/* libcall hashtable helpers. */
>>> +
>>> +struct libcall_hasher : typed_noop_remove
>>> {
>>> - return rtx_equal_p ((const_rtx) p1, (const_rtx) p2);
>>> + typedef rtx_def value_type;
>>> + typedef rtx_def compare_type;
>>> + static inline hashval_t hash (const value_type *);
>>> + static inline bool equal (const value_type *, const compare_type *);
>>> + static inline void remove (value_type *);
>>> +};
>>> +
>>> +inline bool
>>> +libcall_hasher::equal (const value_type *p1, const compare_type *p2)
>>> +{
>>> + return rtx_equal_p (p1, p2);
>>> }
>>>
>>> -static hashval_t
>>> -libcall_hash (const void *p1)
>>> +inline hashval_t
>>> +libcall_hasher::hash (const value_type *p1)
>>> {
>>> - return hash_rtx ((const_rtx) p1, VOIDmode, NULL, NULL, FALSE);
>>> + return hash_rtx (p1, VOIDmode, NULL, NULL, FALSE);
>>> }
>>>
>>> +typedef hash_table libcall_table_type;
>>> +
>>> static void
>>> -add_libcall (htab_t htab, rtx libcall)
>>> +add_libcall (libcall_table_type htab, rtx libcall)
>>> {
>>> - *htab_find_slot (htab, libcall, INSERT) = libcall;
>>> + *htab.find_slot (libcall, INSERT) = libcall;
>>> }
>>>
>>> static bool
>>> arm_libcall_uses_aapcs_base (const_rtx libcall)
>>> {
>>>static bool init_done = false;
>>> - static htab_t libcall_htab;
Re: [patch] std::unique_ptr improvements
On 1/1/13, Geoffrey Romer wrote: > AFAICT there's no way to distinguish between safe and unsafe > conversions of user-defined pointers, because that's a property > of the pointer implementation, not the type itself. My PR errs on > the side of trusting the implementation to provide only correct > conversions. As Jonathan notes, Howard has objected to that part > of the PR, so it's possible the eventual resolution will differ > in that respect; I intend to pick up that discussion next week > when I'm back from vacation. BTW, I've attached my latest set of tests. -- Lawrence Crowl tests.tar.gz Description: GNU Zip compressed data
[google 4_7] backport "std::unique_ptr improvements"
ite/20_util/unique_ptr/cons/pointer_array_convertible_neg.cc
(revision
194742)
+++
libstdc++-v3/testsuite/20_util/unique_ptr/cons/pointer_array_convertible_neg.cc
(working
copy)
@@ -1,7 +1,7 @@
// { dg-do compile }
-// { dg-options "-std=gnu++0x" }
+// { dg-options "-std=gnu++11" }
-// Copyright (C) 2008, 2009 Free Software Foundation
+// Copyright (C) 2008-2012 Free Software Foundation
//
// This file is part of the GNU ISO C++ Library. This library is free
// software; you can redistribute it and/or modify it under the
@@ -29,7 +29,7 @@ struct B : A
virtual ~B() { }
};
-// 20.4.5.1 unique_ptr constructors [unique.ptr.cons]
+// 20.7.1.3.1 unique_ptr constructors [unique.ptr.runtime.ctor]
// Construction from pointer of derived type
void
Index: libstdc++-v3/testsuite/20_util/unique_ptr/requirements/pointer_type.cc
===
--- libstdc++-v3/testsuite/20_util/unique_ptr/requirements/pointer_type.cc
(revision
194742)
+++ libstdc++-v3/testsuite/20_util/unique_ptr/requirements/pointer_type.cc
(working
copy)
@@ -1,7 +1,7 @@
// { dg-do compile }
-// { dg-options "-std=gnu++0x" }
+// { dg-options "-std=gnu++11" }
-// Copyright (C) 2010, 2011 Free Software Foundation
+// Copyright (C) 2010-2012 Free Software Foundation
//
// This file is part of the GNU ISO C++ Library. This library is free
// software; you can redistribute it and/or modify it under the
@@ -18,10 +18,9 @@
// with this library; see the file COPYING3. If not see
// <http://www.gnu.org/licenses/>.
-// 20.6.11 Template class unique_ptr [unique.ptr.single]
+// 20.7.1.2 unique_ptr for single objects [unique.ptr.single]
#include
-#include
struct A
{
Index: libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
===
--- libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
(revision
194742)
+++ libstdc++-v3/testsuite/20_util/unique_ptr/assign/48635_neg.cc
(working copy)
@@ -41,10 +41,10 @@ void f()
std::unique_ptr ub(nullptr, b);
std::unique_ptr ud(nullptr, d);
ub = std::move(ud);
-// { dg-error "use of deleted function" "" { target *-*-* } 195 }
+// { dg-error "use of deleted function" "" { target *-*-* } 203 }
std::unique_ptr uba(nullptr, b);
std::unique_ptr uda(nullptr, d);
uba = std::move(uda);
-// { dg-error "use of deleted function" "" { target *-*-* } 341 }
+// { dg-error "use of deleted function" "" { target *-*-* } 393 }
}
Index: libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc
===
--- libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc (revision
194742)
+++ libstdc++-v3/testsuite/20_util/default_delete/48631_neg.cc (working copy)
@@ -27,4 +27,4 @@ struct D : B { };
D d;
std::default_delete db;
typedef decltype(db(&d)) type; // { dg-error "use of deleted function" }
-// { dg-error "declared here" "" { target *-*-* } 83 }
+// { dg-error "declared here" "" { target *-*-* } 100 }
Index: libstdc++-v3/testsuite/20_util/declval/requirements/1_neg.cc
===
--- libstdc++-v3/testsuite/20_util/declval/requirements/1_neg.cc
(revision
194742)
+++ libstdc++-v3/testsuite/20_util/declval/requirements/1_neg.cc
(working copy)
@@ -19,7 +19,7 @@
// with this library; see the file COPYING3. If not see
// <http://www.gnu.org/licenses/>.
-// { dg-error "static assertion failed" "" { target *-*-* } 1776 }
+// { dg-error "static assertion failed" "" { target *-*-* } 1778 }
#include
--
Lawrence Crowl
Re: [patch] std::unique_ptr improvements
On 1/1/13, Jonathan Wakely wrote: > On 1 January 2013 20:40, Lawrence Crowl wrote: >> That was pilot error on my part. However, I've been having trouble >> when the argument to the constructor or reset has a conversion >> operator. The code does distinquish between a safe conversion to >> base and an unsafe conversion to derived. >> >> Here is a simplified version of the problem. The code as is fails >> to reject the last two calls to accept. The primary reason is that >> is_convertable permits both the invocation of the conversion operator >> and the derived to base conversion. At present, I see no way to >> get a handle on the 'natural' return type of the conversion operator. > > Is the issue here that Geoffrey's proposed resolution allows any > conversions (safe or not) if the source type is not a built-in > pointer, i.e. is some user-defined type? So a user-defined type that > refers an array of derived classes is allowed to be converted to an > array of base, even though that's not safe. Howard has strongly > objected to this part of the P/R. Yes. I see no way to distinguish between objects with safe conversion operators and unsafe conversion operators. -- Lawrence Crowl
Re: [patch] std::unique_ptr improvements
On 12/28/12, Jonathan Wakely wrote:
> On 28 December 2012 01:51, Lawrence Crowl wrote:
>> I'm not getting errors when converting from derived to base.
>> E.g. the following compiles, when it should not.
>>
>> std::unique_ptr acb_ad(new derived[3]);
>
> I get an error:
>
> shm$ cat up.cc
> #include
> struct base { };
> struct derived : base { virtual ~derived() = default; };
> std::unique_ptr acb_ad(new derived[3]);
> shm$
> shm$ g++11 up.cc -c
> up.cc:4:53: error: use of deleted function ‘std::unique_ptr<_Tp [],
> _Dp>::unique_ptr(_Up*) [with _Up = derived; =
> void; _Tp = const base; _Dp = std::default_delete]’
> std::unique_ptr acb_ad(new derived[3]);
> ^
> In file included from /home/redi/gcc/4.x/include/c++/4.8.0/memory:81:0,
> from up.cc:1:
> /home/redi/gcc/4.x/include/c++/4.8.0/bits/unique_ptr.h:343:2: error:
> declared here
> unique_ptr(_Up* __p) = delete;
> ^
That was pilot error on my part. However, I've been having trouble
when the argument to the constructor or reset has a conversion
operator. The code does distinquish between a safe conversion to
base and an unsafe conversion to derived.
Here is a simplified version of the problem. The code as is fails
to reject the last two calls to accept. The primary reason is that
is_convertable permits both the invocation of the conversion operator
and the derived to base conversion. At present, I see no way to
get a handle on the 'natural' return type of the conversion operator.
#include
struct base { };
struct derived : base { };
template
typename std::enable_if<
std::is_convertible< F, T* >::value,
T*
>::type
accept(F arg) { return arg; }
template
typename std::enable_if<
!std::is_convertible< F(*)[], T(*)[] >::value,
T*
>::type
accept(F* arg) = delete;
struct cvt_b {
operator base*() { return 0; }
};
struct cvt_d {
operator derived*() { return 0; }
};
int main()
{
// should PASS
accept< base >( (base*)0 );
accept< const base >( (base*)0 );
accept< base >( cvt_b() );
accept< const base >( cvt_b() );
// should FAIL
accept< base >( (derived*)0 );
accept< const base >( (derived*)0 );
accept< base >( cvt_d() );
accept< const base >( cvt_d() );
}
--
Lawrence Crowl
Re: [patch] std::unique_ptr improvements
On 12/20/12, Jonathan Wakely wrote: > This patch started when I noticed that it's not possibly to construct > a shared_ptr from unique_ptr, then I discovered we don't > use D::pointer if it exists, and there were a number of other > non-conformance issues with our std::unique_ptr. I ended up > fixing them by implementing Geoffrey's proposed resolution for LWG > issue 2118, which isn't official yet but is better than what we had > before so is a step in the right direction, even if it ends up needing > further revision when 2118 is resolved. > > * include/std/functional (_Require): Move to ... > * include/std/type_traits (_Require): ... here. > * include/bits/shared_ptr_base.h > (__shared_count::_S_create_from_up): > Handle unique_ptr for arrays or with custom pointer types. > (__shared_ptr::__shared_ptr(unique_ptr<_Tp1, _Del>&&): Likewise. > * include/bits/unique_ptr.h (unique_ptr<_Tp[], _Dp>): Use > _Dp::pointer if defined. Implement proposed resolution of LWG 2118. > * testsuite/20_util/shared_ptr/cons/unique_ptr_array.cc: New. > * testsuite/20_util/unique_ptr/assign/cv_qual.cc: New. > * testsuite/20_util/unique_ptr/cons/array_convertible_neg.cc: New. > * testsuite/20_util/unique_ptr/cons/convertible_neg.cc: New. > * testsuite/20_util/unique_ptr/cons/cv_qual.cc: New. > * testsuite/20_util/unique_ptr/modifiers/cv_qual.cc: New. > * testsuite/20_util/unique_ptr/requirements/pointer_type_array.cc: > New. > * testsuite/20_util/shared_ptr/cons/unique_ptr.cc: Adjust comments. > * > testsuite/20_util/unique_ptr/cons/pointer_array_convertible_neg.cc: > Likewise. > * testsuite/20_util/unique_ptr/requirements/pointer_type.cc: > Likewise. > * testsuite/20_util/bind/ref_neg.cc: Adjust dg-error line number. > * testsuite/20_util/declval/requirements/1_neg.cc: Likewise. > * testsuite/20_util/default_delete/48631_neg.cc: Likewise. > * testsuite/20_util/shared_ptr/cons/43820_neg.cc: Likewise. > * testsuite/20_util/unique_ptr/assign/48635_neg.cc: Likewise. > * testsuite/20_util/unique_ptr/modifiers/reset_neg.cc: Adjust > dg-error text. > * testsuite/20_util/unique_ptr/cons/ptr_deleter_neg.cc: Use > different instantiations so static_assert fails for each. > > Thanks to Geoffrey and Lawrence for input and test cases. > > Tested x86_64-linux, committed to trunk. I'm not getting errors when converting from derived to base. E.g. the following compiles, when it should not. std::unique_ptr acb_ad(new derived[3]); -- Lawrence Crowl
[cxx-conversion] Change uses of htab_t in gcc/config to hash_table.
nclude "hashtab.h"
+#include "hash-table.h"
#include "debug.h"
#include "target.h"
#include "target-def.h"
@@ -15592,30 +15592,43 @@ mips_hash_base (rtx base)
return hash_rtx (base, GET_MODE (base), &do_not_record_p, NULL, false);
}
+/* Hashtable helpers. */
+
+struct mips_lo_sum_offset_hasher : typed_free_remove
+{
+ typedef mips_lo_sum_offset value_type;
+ typedef rtx_def compare_type;
+ static inline hashval_t hash (const value_type *);
+ static inline bool equal (const value_type *, const compare_type *);
+};
+
/* Hash-table callbacks for mips_lo_sum_offsets. */
-static hashval_t
-mips_lo_sum_offset_hash (const void *entry)
+inline hashval_t
+mips_lo_sum_offset_hasher::hash (const value_type *entry)
{
- return mips_hash_base (((const struct mips_lo_sum_offset *) entry)->base);
+ return mips_hash_base (entry->base);
}
-static int
-mips_lo_sum_offset_eq (const void *entry, const void *value)
+inline bool
+mips_lo_sum_offset_hasher::equal (const value_type *entry,
+ const compare_type *value)
{
- return rtx_equal_p (((const struct mips_lo_sum_offset *) entry)->base,
- (const_rtx) value);
+ return rtx_equal_p (entry->base, value);
}
+typedef hash_table mips_offset_table;
+
/* Look up symbolic constant X in HTAB, which is a hash table of
mips_lo_sum_offsets. If OPTION is NO_INSERT, return true if X can be
paired with a recorded LO_SUM, otherwise record X in the table. */
static bool
-mips_lo_sum_offset_lookup (htab_t htab, rtx x, enum insert_option option)
+mips_lo_sum_offset_lookup (mips_offset_table htab, rtx x,
+ enum insert_option option)
{
rtx base, offset;
- void **slot;
+ mips_lo_sum_offset **slot;
struct mips_lo_sum_offset *entry;
/* Split X into a base and offset. */
@@ -15624,7 +15637,7 @@ mips_lo_sum_offset_lookup (htab_t htab,
base = UNSPEC_ADDRESS (base);
/* Look up the base in the hash table. */
- slot = htab_find_slot_with_hash (htab, base, mips_hash_base (base), option);
+ slot = htab.find_slot_with_hash (base, mips_hash_base (base), option);
if (slot == NULL)
return false;
@@ -15654,7 +15667,8 @@ static int
mips_record_lo_sum (rtx *loc, void *data)
{
if (GET_CODE (*loc) == LO_SUM)
-mips_lo_sum_offset_lookup ((htab_t) data, XEXP (*loc, 1), INSERT);
+mips_lo_sum_offset_lookup (*(mips_offset_table*)data,
+ XEXP (*loc, 1), INSERT);
return 0;
}
@@ -15663,7 +15677,7 @@ mips_record_lo_sum (rtx *loc, void *data
LO_SUMs in the current function. */
static bool
-mips_orphaned_high_part_p (htab_t htab, rtx insn)
+mips_orphaned_high_part_p (mips_offset_table htab, rtx insn)
{
enum mips_symbol_type type;
rtx x, set;
@@ -15771,7 +15785,7 @@ mips_reorg_process_insns (void)
{
rtx insn, last_insn, subinsn, next_insn, lo_reg, delayed_reg;
int hilo_delay;
- htab_t htab;
+ mips_offset_table htab;
/* Force all instructions to be split into their final form. */
split_all_insns_noflow ();
@@ -15808,14 +15822,13 @@ mips_reorg_process_insns (void)
if (TARGET_FIX_VR4130 && !ISA_HAS_MACCHI)
cfun->machine->all_noreorder_p = false;
- htab = htab_create (37, mips_lo_sum_offset_hash,
- mips_lo_sum_offset_eq, free);
+ htab.create (37);
/* Make a first pass over the instructions, recording all the LO_SUMs. */
for (insn = get_insns (); insn != 0; insn = NEXT_INSN (insn))
FOR_EACH_SUBINSN (subinsn, insn)
if (USEFUL_INSN_P (subinsn))
- for_each_rtx (&PATTERN (subinsn), mips_record_lo_sum, htab);
+ for_each_rtx (&PATTERN (subinsn), mips_record_lo_sum, &htab);
last_insn = 0;
hilo_delay = 2;
@@ -15872,7 +15885,7 @@ mips_reorg_process_insns (void)
}
}
- htab_delete (htab);
+ htab.dispose ();
}
/* Return true if the function has a long branch instruction. */
--
Lawrence Crowl
Re: [PATCH] Fix up sbitmap bitmap_{and,ior,xor} (PR target/55562)
On 12/18/12, Jakub Jelinek wrote:
> The bitmap unification changes apparently broke at least modulo
> scheduling, which used sbitmap_a_and_b_cg functions, which
> were replaced by bitmap_and. But, sbitmap_a_and_b_cg asserted
> dst->popcount is NULL and returned whether dst sbitmap changed,
> but bitmap_and returns always false if dst->popcount is NULL
> (and only if it is non-NULL returns if the bitmap changed).
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?
The changes look okay to me. (But I don't approve trunk.)
> Alternatively we could add back separate functions that would
> return whether dest changed for speed reasons (similarly to
> the old *_cg ones), and return void from the normal ones again.
> I bet most of the users don't check the return value of these
> functions and thus it is useless computation.
The discussion at the time was pretty conclusive to not have
separate functions.
>
> 2012-12-18 Jakub Jelinek
>
> PR target/55562
> * sbitmap.c (bitmap_and, bitmap_xor, bitmap_ior): Return whether
> dst sbitmap changed even if it doesn't have popcount.
>
> --- gcc/sbitmap.c.jj 2012-11-02 09:01:54.0 +0100
> +++ gcc/sbitmap.c 2012-12-18 14:29:13.695299294 +0100
> @@ -434,28 +434,26 @@ bitmap_and (sbitmap dst, const_sbitmap a
>const_sbitmap_ptr bp = b->elms;
>bool has_popcount = dst->popcount != NULL;
>unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
>for (i = 0; i < n; i++)
> {
>const SBITMAP_ELT_TYPE tmp = *ap++ & *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
>if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
>*dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
>if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Set DST to be (A xor B)).
> @@ -470,28 +468,26 @@ bitmap_xor (sbitmap dst, const_sbitmap a
>const_sbitmap_ptr bp = b->elms;
>bool has_popcount = dst->popcount != NULL;
>unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
>for (i = 0; i < n; i++)
> {
>const SBITMAP_ELT_TYPE tmp = *ap++ ^ *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
>if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
>*dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
>if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Set DST to be (A or B)).
> @@ -506,28 +502,26 @@ bitmap_ior (sbitmap dst, const_sbitmap a
>const_sbitmap_ptr bp = b->elms;
>bool has_popcount = dst->popcount != NULL;
>unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
>for (i = 0; i < n; i++)
> {
>const SBITMAP_ELT_TYPE tmp = *ap++ | *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
>if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
>*dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
>if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Return nonzero if A is a subset of B. */
--
Lawrence Crowl
Re: [cxx-conversion] Change tree-ssa-coalesce.c'coalesce_list_d.list to hash_table
On 12/17/12, Richard Biener wrote:
> On Mon, Dec 17, 2012 at 8:36 PM, Lawrence Crowl wrote:
>> Change tree-ssa-coalesce.c'coalesce_list_d.list from htab_t to
>> hash_table.
>>
>> Fold coalesce_pair_map_hash and coalesce_pair_map_eq into new
>> struct coalesce_pair_hasher.
>>
>> Removed struct coalesce_pair_iterator, as did not meet the hash_table
>> iterator interface and it provided no significant code reduction.
>>
>> Tested on x86-64.
>>
>> Okay for branch?
>>
>>
>> Index: gcc/tree-ssa-coalesce.c
>> ===
>> --- gcc/tree-ssa-coalesce.c (revision 194549)
>> +++ gcc/tree-ssa-coalesce.c (working copy)
>> @@ -50,6 +50,41 @@ typedef struct coalesce_pair
>> } * coalesce_pair_p;
>> typedef const struct coalesce_pair *const_coalesce_pair_p;
>>
>> +/* Coalesce pair hashtable helpers. */
>> +
>> +struct coalesce_pair_hasher : typed_noop_remove
>> +{
>> + typedef coalesce_pair value_type;
>> + typedef coalesce_pair compare_type;
>> + static inline hashval_t hash (const value_type *);
>> + static inline bool equal (const value_type *, const compare_type *);
>> +};
>> +
>> +/* Hash function for coalesce list. Calculate hash for PAIR. */
>> +
>> +inline hashval_t
>> +coalesce_pair_hasher::hash (const value_type *pair)
>> +{
>> + hashval_t a = (hashval_t)(pair->first_element);
>> + hashval_t b = (hashval_t)(pair->second_element);
>> +
>> + return b * (b - 1) / 2 + a;
>> +}
>> +
>> +/* Equality function for coalesce list hash table. Compare PAIR1 and
>> PAIR2,
>> + returning TRUE if the two pairs are equivalent. */
>> +
>> +inline bool
>> +coalesce_pair_hasher::equal (const value_type *p1, const compare_type
>> *p2)
>> +{
>> + return (p1->first_element == p2->first_element
>> + && p1->second_element == p2->second_element);
>> +}
>> +
>> +typedef hash_table coalesce_table_type;
>> +typedef coalesce_table_type::iterator coalesce_iterator_type;
>> +
>> +
>> typedef struct cost_one_pair_d
>> {
>>int first_element;
>> @@ -61,7 +96,7 @@ typedef struct cost_one_pair_d
>>
>> typedef struct coalesce_list_d
>> {
>> - htab_t list; /* Hash table. */
>> + coalesce_table_type list;/* Hash table. */
>>coalesce_pair_p *sorted; /* List when sorted. */
>>int num_sorted; /* Number in the sorted list. */
>>cost_one_pair_p cost_one_list;/* Single use coalesces with cost 1. */
>> @@ -186,34 +221,6 @@ pop_best_coalesce (coalesce_list_p cl, i
>> }
>>
>>
>> -#define COALESCE_HASH_FN(R1, R2) ((R2) * ((R2) - 1) / 2 + (R1))
>> -
>> -/* Hash function for coalesce list. Calculate hash for PAIR. */
>> -
>> -static unsigned int
>> -coalesce_pair_map_hash (const void *pair)
>> -{
>> - hashval_t a =
>> (hashval_t)(((const_coalesce_pair_p)pair)->first_element);
>> - hashval_t b =
>> (hashval_t)(((const_coalesce_pair_p)pair)->second_element);
>> -
>> - return COALESCE_HASH_FN (a,b);
>> -}
>> -
>> -
>> -/* Equality function for coalesce list hash table. Compare PAIR1 and
>> PAIR2,
>> - returning TRUE if the two pairs are equivalent. */
>> -
>> -static int
>> -coalesce_pair_map_eq (const void *pair1, const void *pair2)
>> -{
>> - const_coalesce_pair_p const p1 = (const_coalesce_pair_p) pair1;
>> - const_coalesce_pair_p const p2 = (const_coalesce_pair_p) pair2;
>> -
>> - return (p1->first_element == p2->first_element
>> - && p1->second_element == p2->second_element);
>> -}
>> -
>> -
>> /* Create a new empty coalesce list object and return it. */
>>
>> static inline coalesce_list_p
>> @@ -226,8 +233,7 @@ create_coalesce_list (void)
>> size = 40;
>>
>>list = (coalesce_list_p) xmalloc (sizeof (struct coalesce_list_d));
>> - list->list = htab_create (size, coalesce_pair_map_hash,
>> - coalesce_pair_map_eq, NULL);
>> + list->list.create (size);
>>list->sorted = NULL;
>>list->num_sorted = 0;
>>list->cost_one_list = NULL;
>> @@ -241,7 +247,7 @@ static inline void
>> delete_coalesce_list (coalesce_list_p cl)
>> {
>>gcc_assert (cl->cost_one_list == NULL);
>> - htab_delete (cl->list);
>> + cl-&
[cxx-conversion] Change tree-ssa-coalesce.c'coalesce_list_d.list to hash_table
eturn htab_elements (cl->list);
-}
-
-
-/* Iterator over hash table pairs. */
-typedef struct
-{
- htab_iterator hti;
-} coalesce_pair_iterator;
-
-
-/* Return first partition pair from list CL, initializing iterator ITER. */
-
-static inline coalesce_pair_p
-first_coalesce_pair (coalesce_list_p cl, coalesce_pair_iterator *iter)
-{
- coalesce_pair_p pair;
-
- pair = (coalesce_pair_p) first_htab_element (&(iter->hti), cl->list);
- return pair;
-}
-
-
-/* Return TRUE if there are no more partitions in for ITER to process. */
-
-static inline bool
-end_coalesce_pair_p (coalesce_pair_iterator *iter)
-{
- return end_htab_p (&(iter->hti));
-}
-
-
-/* Return the next partition pair to be visited by ITER. */
-
-static inline coalesce_pair_p
-next_coalesce_pair (coalesce_pair_iterator *iter)
-{
- coalesce_pair_p pair;
-
- pair = (coalesce_pair_p) next_htab_element (&(iter->hti));
- return pair;
+ return cl->list.elements ();
}
-/* Iterate over CL using ITER, returning values in PAIR. */
-
-#define FOR_EACH_PARTITION_PAIR(PAIR, ITER, CL)\
- for ((PAIR) = first_coalesce_pair ((CL), &(ITER)); \
- !end_coalesce_pair_p (&(ITER)); \
- (PAIR) = next_coalesce_pair (&(ITER)))
-
-
/* Prepare CL for removal of preferred pairs. When finished they are sorted
in order from most important coalesce to least important. */
@@ -416,7 +373,7 @@ sort_coalesce_list (coalesce_list_p cl)
{
unsigned x, num;
coalesce_pair_p p;
- coalesce_pair_iterator ppi;
+ coalesce_iterator_type ppi;
gcc_assert (cl->sorted == NULL);
@@ -430,7 +387,7 @@ sort_coalesce_list (coalesce_list_p cl)
/* Populate the vector with pointers to the pairs. */
x = 0;
- FOR_EACH_PARTITION_PAIR (p, ppi, cl)
+ FOR_EACH_HASH_TABLE_ELEMENT (cl->list, p, coalesce_pair_p, ppi)
cl->sorted[x++] = p;
gcc_assert (x == num);
@@ -462,14 +419,15 @@ static void
dump_coalesce_list (FILE *f, coalesce_list_p cl)
{
coalesce_pair_p node;
- coalesce_pair_iterator ppi;
+ coalesce_iterator_type ppi;
+
int x;
tree var;
if (cl->sorted == NULL)
{
fprintf (f, "Coalesce List:\n");
- FOR_EACH_PARTITION_PAIR (node, ppi, cl)
+ FOR_EACH_HASH_TABLE_ELEMENT (cl->list, node, coalesce_pair_p, ppi)
{
tree var1 = ssa_name (node->first_element);
tree var2 = ssa_name (node->second_element);
--
Lawrence Crowl
[cxx-conversion] Convert remaining tree-parloops.c htab_t to hash_table.
t)
{
edge exit = single_dom_exit (loop);
gimple_stmt_iterator gsi;
@@ -2036,7 +2054,7 @@ try_create_reduction_list (loop_p loop,
fprintf (dump_file,
" checking if it a part of reduction pattern: \n");
}
- if (htab_elements (reduction_list) == 0)
+ if (reduction_list.elements () == 0)
{
if (dump_file && (dump_flags & TDF_DETAILS))
fprintf (dump_file,
@@ -2110,7 +2128,7 @@ parallelize_loops (void)
struct loop *loop;
struct tree_niter_desc niter_desc;
loop_iterator li;
- htab_t reduction_list;
+ reduction_info_table_type reduction_list;
struct obstack parloop_obstack;
HOST_WIDE_INT estimated;
LOC loop_loc;
@@ -2122,13 +2140,12 @@ parallelize_loops (void)
return false;
gcc_obstack_init (&parloop_obstack);
- reduction_list = htab_create (10, reduction_info_hash,
-reduction_info_eq, free);
+ reduction_list.create (10);
init_stmt_vec_info_vec ();
FOR_EACH_LOOP (li, loop, 0)
{
- htab_empty (reduction_list);
+ reduction_list.empty ();
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, "Trying loop %d as candidate\n",loop->num);
@@ -2208,7 +2225,7 @@ parallelize_loops (void)
}
free_stmt_vec_info_vec ();
- htab_delete (reduction_list);
+ reduction_list.dispose ();
obstack_free (&parloop_obstack, NULL);
/* Parallelization will cause new function calls to be inserted through
--
Lawrence Crowl
[cxx-conversion] Convert statistics.c'statistics_hashes to hash_table.
void **slot, voi
/* Helper for statistics_fini_pass, reset the counters. */
-static int
-statistics_fini_pass_3 (void **slot, void *data ATTRIBUTE_UNUSED)
+int
+statistics_fini_pass_3 (statistics_counter_t **slot,
+ void *data ATTRIBUTE_UNUSED)
{
- statistics_counter_t *counter = (statistics_counter_t *)*slot;
+ statistics_counter_t *counter = *slot;
counter->prev_dumped_count = counter->count;
return 1;
}
@@ -180,26 +193,25 @@ statistics_fini_pass (void)
fprintf (dump_file, "\n");
fprintf (dump_file, "Pass statistics:\n");
fprintf (dump_file, "\n");
- htab_traverse_noresize (curr_statistics_hash (),
- statistics_fini_pass_1, NULL);
+ curr_statistics_hash ()
+ .traverse_noresize (NULL);
fprintf (dump_file, "\n");
}
if (statistics_dump_file
&& !(statistics_dump_flags & TDF_STATS
|| statistics_dump_flags & TDF_DETAILS))
-htab_traverse_noresize (curr_statistics_hash (),
- statistics_fini_pass_2, NULL);
- htab_traverse_noresize (curr_statistics_hash (),
- statistics_fini_pass_3, NULL);
+curr_statistics_hash ()
+ .traverse_noresize (NULL);
+ curr_statistics_hash ()
+.traverse_noresize (NULL);
}
/* Helper for printing summary information. */
-static int
-statistics_fini_1 (void **slot, void *data)
+int
+statistics_fini_1 (statistics_counter_t **slot, opt_pass *pass)
{
- struct opt_pass *pass = (struct opt_pass *)data;
- statistics_counter_t *counter = (statistics_counter_t *)*slot;
+ statistics_counter_t *counter = *slot;
if (counter->count == 0)
return 1;
if (counter->histogram_p)
@@ -231,10 +243,11 @@ statistics_fini (void)
{
unsigned i;
for (i = 0; i < nr_statistics_hashes; ++i)
- if (statistics_hashes[i] != NULL
+ if (statistics_hashes[i].is_created ()
&& get_pass_for_id (i) != NULL)
- htab_traverse_noresize (statistics_hashes[i],
- statistics_fini_1, get_pass_for_id (i));
+ statistics_hashes[i]
+ .traverse_noresize
+ (get_pass_for_id (i));
}
dump_end (statistics_dump_nr, statistics_dump_file);
@@ -262,14 +275,14 @@ statistics_init (void)
and HISTOGRAM_P. */
static statistics_counter_t *
-lookup_or_add_counter (htab_t hash, const char *id, int val,
+lookup_or_add_counter (stats_counter_table_type hash, const char *id, int val,
bool histogram_p)
{
statistics_counter_t **counter;
statistics_counter_t c;
c.id = id;
c.val = val;
- counter = (statistics_counter_t **) htab_find_slot (hash, &c, INSERT);
+ counter = hash.find_slot (&c, INSERT);
if (!*counter)
{
*counter = XNEW (struct statistics_counter_s);
--
Lawrence Crowl
Re: [cxx-conversion] Convert tree-sra.c'candidates to hash_table
On 12/13/12, Martin Jambor wrote:
> On Thu, Dec 13, 2012 at 11:05:49AM -0800, Lawrence Crowl wrote:
> > On 12/12/12, Jakub Jelinek wrote:
> > > On Tue, Dec 11, 2012 at 02:44:41PM -0800, Lawrence Crowl wrote:
> > > > +/* Hash a tree in a uid_decl_map. */
> > > > +
> > > > +inline hashval_t
> > > > +uid_decl_hasher::hash (const value_type *item)
> > > > +{
> > > > + return item->decl_minimal.uid;
> > >
> > > Ugh, why aren't you using DECL_UID here? The fact that DECL_UID is
> > > right now defined to (DECL_MINIMAL_CHECK (NODE)->decl_minimal.uid)
> > > might change, we really don't want to update thousands of locations
> > > should we need to change that.
> >
> > That is what the code looked like in its original location. I have
> > resisted making changes not essential to the purpose of the patch.
> > However, if you want me to change it to use DECL_UID, I will.
>
> Yes, I think it ought to be changed.
> Thanks for the conversion,
It turns out that making the change causes the check to fail.
Therefore, I will leave the code alone and let someone more familiar
with the code investigate.
--
Lawrence Crowl
Re: [cxx-conversion] Convert tree-sra.c'candidates to hash_table
On 12/12/12, Jakub Jelinek wrote:
> On Tue, Dec 11, 2012 at 02:44:41PM -0800, Lawrence Crowl wrote:
> > +/* Hash a tree in a uid_decl_map. */
> > +
> > +inline hashval_t
> > +uid_decl_hasher::hash (const value_type *item)
> > +{
> > + return item->decl_minimal.uid;
>
> Ugh, why aren't you using DECL_UID here? The fact that DECL_UID is
> right now defined to (DECL_MINIMAL_CHECK (NODE)->decl_minimal.uid)
> might change, we really don't want to update thousands of locations
> should we need to change that.
That is what the code looked like in its original location. I have
resisted making changes not essential to the purpose of the patch.
However, if you want me to change it to use DECL_UID, I will.
--
Lawrence Crowl
Re: [cxx-conversion] Convert tree-sra.c'candidates to hash_table
On 12/12/12, Diego Novillo wrote:
> On Dec 11, 2012 Lawrence Crowl wrote:
> > Convert tree-sra.c'candidates from htab_t to hash_table.
> >
> > Fold uid_decl_map_hash and uid_decl_map_eq into new struct
> > uid_decl_hasher. This change moves the definitions from tree-ssa.c
> > into tree-sra.c and removes the declarations from tree-flow.h
> >
> > Update dependent calls and types to hash_table.
> >
> > Tested on x86_64.
> >
> > Okay for branch?
> >
> >
> > Index: gcc/tree-sra.c
> > +/* Candidate ashtable helpers. */
>
> s/ashtable/hash table/
>
> > +struct uid_decl_hasher : typed_noop_remove
> > +{
> > + typedef tree_node value_type;
> > + typedef tree_node compare_type;
> > + static inline hashval_t hash (const value_type *);
> > + static inline bool equal (const value_type *, const compare_type *);
> > +};
> > +
> > +/* Hash a tree in a uid_decl_map. */
> > +
> > +inline hashval_t
> > +uid_decl_hasher::hash (const value_type *item)
> > +{
> > + return item->decl_minimal.uid;
> > +}
> > +
> > +/* Return true if the DECL_UID in both trees are equal. */
> > +
> > +inline bool
> > +uid_decl_hasher::equal (const value_type *a, const compare_type *b)
> > +{
> > + return (a->decl_minimal.uid == b->decl_minimal.uid);
> > +}
>
> ISTR other places where we hash decls. I wonder if we shouldn't
> move this to some common spot. Maybe later.
Decls are hashed in different ways to match the equality function.
There probably are duplicates in there, but I'm sure they aren't
all duplicates.
> The patch is OK.
--
Lawrence Crowl
[cxx-conversion] Convert tree-vectorizer.h'_loop_vec_info::peeling_htab to hash_table
t->count = 1;
- new_slot = htab_find_slot (LOOP_VINFO_PEELING_HTAB (loop_vinfo), slot,
- INSERT);
+ new_slot = LOOP_VINFO_PEELING_HTAB (loop_vinfo).find_slot (slot, INSERT);
*new_slot = slot;
}
@@ -1330,11 +1307,11 @@ vect_peeling_hash_insert (loop_vec_info
/* Traverse peeling hash table to find peeling option that aligns maximum
number of data accesses. */
-static int
-vect_peeling_hash_get_most_frequent (void **slot, void *data)
+int
+vect_peeling_hash_get_most_frequent (_vect_peel_info **slot,
+_vect_peel_extended_info *max)
{
- vect_peel_info elem = (vect_peel_info) *slot;
- vect_peel_extended_info max = (vect_peel_extended_info) data;
+ vect_peel_info elem = *slot;
if (elem->count > max->peel_info.count
|| (elem->count == max->peel_info.count
@@ -1352,11 +1329,11 @@ vect_peeling_hash_get_most_frequent (voi
/* Traverse peeling hash table and calculate cost for each peeling option.
Find the one with the lowest cost. */
-static int
-vect_peeling_hash_get_lowest_cost (void **slot, void *data)
+int
+vect_peeling_hash_get_lowest_cost (_vect_peel_info **slot,
+ _vect_peel_extended_info *min)
{
- vect_peel_info elem = (vect_peel_info) *slot;
- vect_peel_extended_info min = (vect_peel_extended_info) data;
+ vect_peel_info elem = *slot;
int save_misalignment, dummy;
unsigned int inside_cost = 0, outside_cost = 0, i;
gimple stmt = DR_STMT (elem->dr);
@@ -1436,14 +1413,16 @@ vect_peeling_hash_choose_best_peeling (l
{
res.inside_cost = INT_MAX;
res.outside_cost = INT_MAX;
- htab_traverse (LOOP_VINFO_PEELING_HTAB (loop_vinfo),
- vect_peeling_hash_get_lowest_cost, &res);
+ LOOP_VINFO_PEELING_HTAB (loop_vinfo)
+ .traverse <_vect_peel_extended_info *,
+ vect_peeling_hash_get_lowest_cost> (&res);
}
else
{
res.peel_info.count = 0;
- htab_traverse (LOOP_VINFO_PEELING_HTAB (loop_vinfo),
- vect_peeling_hash_get_most_frequent, &res);
+ LOOP_VINFO_PEELING_HTAB (loop_vinfo)
+ .traverse <_vect_peel_extended_info *,
+ vect_peeling_hash_get_most_frequent> (&res);
}
*npeel = res.peel_info.npeel;
@@ -1648,10 +1627,8 @@ vect_enhance_data_refs_alignment (loop_v
size_zero_node) < 0;
/* Save info about DR in the hash table. */
- if (!LOOP_VINFO_PEELING_HTAB (loop_vinfo))
-LOOP_VINFO_PEELING_HTAB (loop_vinfo) =
- htab_create (1, vect_peeling_hash,
-vect_peeling_hash_eq, free);
+ if (!LOOP_VINFO_PEELING_HTAB (loop_vinfo).is_created ())
+LOOP_VINFO_PEELING_HTAB (loop_vinfo).create (1);
vectype = STMT_VINFO_VECTYPE (stmt_info);
nelements = TYPE_VECTOR_SUBPARTS (vectype);
--
Lawrence Crowl
[cxx-conversion] Convert various htab_t tables to hash_table
e = get_addr_base_and_unit_offset (exp, &off);
@@ -154,8 +171,7 @@ get_addr_stridx (tree exp)
return 0;
ent.base.from = base;
- e = (struct decl_stridxlist_map *)
- htab_find_with_hash (decl_to_stridxlist_htab, &ent, DECL_UID (base));
+ e = decl_to_stridxlist_htab.find_with_hash (&ent, DECL_UID (base));
if (e == NULL)
return 0;
@@ -234,7 +250,7 @@ unshare_strinfo_vec (void)
static int *
addr_stridxptr (tree exp)
{
- void **slot;
+ decl_stridxlist_map **slot;
struct decl_stridxlist_map ent;
struct stridxlist *list;
HOST_WIDE_INT off;
@@ -243,19 +259,18 @@ addr_stridxptr (tree exp)
if (base == NULL_TREE || !DECL_P (base))
return NULL;
- if (decl_to_stridxlist_htab == NULL)
+ if (!decl_to_stridxlist_htab.is_created ())
{
- decl_to_stridxlist_htab
- = htab_create (64, decl_to_stridxlist_hash, tree_map_base_eq, NULL);
+ decl_to_stridxlist_htab.create (64);
gcc_obstack_init (&stridx_obstack);
}
ent.base.from = base;
- slot = htab_find_slot_with_hash (decl_to_stridxlist_htab, &ent,
- DECL_UID (base), INSERT);
+ slot = decl_to_stridxlist_htab.find_slot_with_hash (&ent, DECL_UID (base),
+ INSERT);
if (*slot)
{
int i;
- list = &((struct decl_stridxlist_map *)*slot)->list;
+ list = &(*slot)->list;
for (i = 0; i < 16; i++)
{
if (list->offset == off)
@@ -273,7 +288,7 @@ addr_stridxptr (tree exp)
struct decl_stridxlist_map *e
= XOBNEW (&stridx_obstack, struct decl_stridxlist_map);
e->base.from = base;
- *slot = (void *) e;
+ *slot = e;
list = &e->list;
}
list->next = NULL;
@@ -1987,11 +2002,10 @@ tree_ssa_strlen (void)
ssa_ver_to_stridx.release ();
free_alloc_pool (strinfo_pool);
- if (decl_to_stridxlist_htab)
+ if (decl_to_stridxlist_htab.is_created ())
{
obstack_free (&stridx_obstack, NULL);
- htab_delete (decl_to_stridxlist_htab);
- decl_to_stridxlist_htab = NULL;
+ decl_to_stridxlist_htab.dispose ();
}
laststmt.stmt = NULL;
laststmt.len = NULL_TREE;
Index: gcc/tree-ssa-loop-ivopts.c
===
--- gcc/tree-ssa-loop-ivopts.c (revision 194381)
+++ gcc/tree-ssa-loop-ivopts.c (working copy)
@@ -76,7 +76,7 @@ along with GCC; see the file COPYING3.
#include "ggc.h"
#include "insn-config.h"
#include "pointer-set.h"
-#include "hashtab.h"
+#include "hash-table.h"
#include "tree-chrec.h"
#include "tree-scalar-evolution.h"
#include "cfgloop.h"
@@ -237,6 +237,33 @@ typedef struct iv_use *iv_use_p;
typedef struct iv_cand *iv_cand_p;
+/* Hashtable helpers. */
+
+struct iv_inv_expr_hasher : typed_free_remove
+{
+ typedef iv_inv_expr_ent value_type;
+ typedef iv_inv_expr_ent compare_type;
+ static inline hashval_t hash (const value_type *);
+ static inline bool equal (const value_type *, const compare_type *);
+};
+
+/* Hash function for loop invariant expressions. */
+
+inline hashval_t
+iv_inv_expr_hasher::hash (const value_type *expr)
+{
+ return expr->hash;
+}
+
+/* Hash table equality function for expressions. */
+
+inline bool
+iv_inv_expr_hasher::equal (const value_type *expr1, const compare_type *expr2)
+{
+ return expr1->hash == expr2->hash
+&& operand_equal_p (expr1->expr, expr2->expr, 0);
+}
+
struct ivopts_data
{
/* The currently optimized loop. */
@@ -256,7 +283,7 @@ struct ivopts_data
/* The hashtable of loop invariant expressions created
by ivopt. */
- htab_t inv_expr_tab;
+ hash_table inv_expr_tab;
/* Loop invariant expression id. */
int inv_expr_id;
@@ -815,30 +842,6 @@ niter_for_single_dom_exit (struct ivopts
return niter_for_exit (data, exit);
}
-/* Hash table equality function for expressions. */
-
-static int
-htab_inv_expr_eq (const void *ent1, const void *ent2)
-{
- const struct iv_inv_expr_ent *expr1 =
- (const struct iv_inv_expr_ent *)ent1;
- const struct iv_inv_expr_ent *expr2 =
- (const struct iv_inv_expr_ent *)ent2;
-
- return expr1->hash == expr2->hash
-&& operand_equal_p (expr1->expr, expr2->expr, 0);
-}
-
-/* Hash function for loop invariant expressions. */
-
-static hashval_t
-htab_inv_expr_hash (const void *ent)
-{
- const struct iv_inv_expr_ent *expr =
- (const struct iv_inv_expr_ent *)ent;
- return expr->hash;
-}
-
/* Initializes data structures used by the iv optimization pass, stored
in DATA. */
@@ -853,8 +856,7 @@ tree_ssa_iv_optimize_init (struct ivopts
data->niters = NULL;
data->iv_uses.create (20);
data->iv_candidates.create (20);
- data->inv_expr_tab = htab_create (10, htab_inv_expr_hash,
-htab_inv_expr_eq, free);
+ data->inv_expr_tab.create (10);
data->inv_expr_id = 0;
decl_rtl_to_reset.create (20);
}
@@ -3806,8 +3808,7 @@ get_expr_id (struct ivopts_data *data, t
ent.expr = expr;
ent.hash = iterative_hash_expr (expr, 0);
- slot = (struct iv_inv_expr_ent **) htab_find_slot (data->inv_expr_tab,
- &ent, INSERT);
+ slot = data->inv_expr_tab.find_slot (&ent, INSERT);
if (*slot)
return (*slot)->id;
@@ -6631,7 +6632,7 @@ free_loop_data (struct ivopts_data *data
decl_rtl_to_reset.truncate (0);
- htab_empty (data->inv_expr_tab);
+ data->inv_expr_tab.empty ();
data->inv_expr_id = 0;
}
@@ -6649,7 +6650,7 @@ tree_ssa_iv_optimize_finalize (struct iv
decl_rtl_to_reset.release ();
data->iv_uses.release ();
data->iv_candidates.release ();
- htab_delete (data->inv_expr_tab);
+ data->inv_expr_tab.dispose ();
}
/* Returns true if the loop body BODY includes any function calls. */
--
Lawrence Crowl
[cxx-conversion] Convert tree-sra.c'candidates to hash_table
onst void *vb)
-{
- const_tree a = (const_tree) va;
- const_tree b = (const_tree) vb;
- return (a->decl_minimal.uid == b->decl_minimal.uid);
-}
-
-/* Hash a tree in a uid_decl_map. */
-
-unsigned int
-uid_decl_map_hash (const void *item)
-{
- return ((const_tree)item)->decl_minimal.uid;
-}
-
-/* Return true if the DECL_UID in both trees are equal. */
-
static int
uid_ssaname_map_eq (const void *va, const void *vb)
{
Index: gcc/tree-flow.h
===
--- gcc/tree-flow.h (revision 194381)
+++ gcc/tree-flow.h (working copy)
@@ -283,9 +283,6 @@ struct int_tree_map {
tree to;
};
-extern unsigned int uid_decl_map_hash (const void *);
-extern int uid_decl_map_eq (const void *, const void *);
-
#define num_ssa_names (vec_safe_length (cfun->gimple_df->ssa_names))
#define ssa_name(i) ((*cfun->gimple_df->ssa_names)[(i)])
Index: gcc/Makefile.in
===
--- gcc/Makefile.in (revision 194381)
+++ gcc/Makefile.in (working copy)
@@ -3034,7 +3034,7 @@ tree-ssa-strlen.o : tree-ssa-strlen.c $(
$(TREE_FLOW_H) $(TREE_PASS_H) domwalk.h alloc-pool.h tree-ssa-propagate.h \
$(GIMPLE_PRETTY_PRINT_H) $(PARAMS_H) $(EXPR_H)
tree-sra.o : tree-sra.c $(CONFIG_H) $(SYSTEM_H) coretypes.h alloc-pool.h \
- $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) \
+ $(HASH_TABLE_H) $(TM_H) $(TREE_H) $(GIMPLE_H) $(CGRAPH_H) $(TREE_FLOW_H) \
$(IPA_PROP_H) $(DIAGNOSTIC_H) statistics.h \
$(PARAMS_H) $(TARGET_H) $(FLAGS_H) \
$(DBGCNT_H) $(TREE_INLINE_H) $(GIMPLE_PRETTY_PRINT_H)
--
Lawrence Crowl
[cxx-conversion] Fix hash_table build problems with checking enabled.
Fix some hash_table build errors when configured with
--enable-checking=yes.
tree-browser.c
* Remove stale declaration of removed TB_parent_eq.
* Fix template parameter for base class to match value_type.
gimple.h
* Use gimplify_hasher::hash rather than gimple_tree_hash in the
assertion check.
* Change return values to match return type. (I.e. no conversions.)
Tested on x86-64.
Committing to branch as obvious.
Index: gcc/tree-browser.c
===
--- gcc/tree-browser.c (revision 194227)
+++ gcc/tree-browser.c (working copy)
@@ -96,14 +96,13 @@ static tree TB_next_expr (tree);
static tree TB_up_expr (tree);
static tree TB_first_in_bind (tree);
static tree TB_last_in_bind (tree);
-static int TB_parent_eq (const void *, const void *);
static tree TB_history_prev (void);
/* FIXME: To be declared in a .h file. */
void browse_tree (tree);
/* Hashtable helpers. */
-struct tree_upper_hasher : typed_noop_remove
+struct tree_upper_hasher : typed_noop_remove
{
typedef tree_node value_type;
typedef tree_node compare_type;
Index: gcc/gimple.h
===
--- gcc/gimple.h(revision 194227)
+++ gcc/gimple.h(working copy)
@@ -972,18 +972,18 @@ gimplify_hasher::equal (const value_type
if (TREE_CODE (t2) != code
|| TREE_TYPE (t1) != TREE_TYPE (t2))
-return 0;
+return false;
if (!operand_equal_p (t1, t2, 0))
-return 0;
+return false;
#ifdef ENABLE_CHECKING
/* Only allow them to compare equal if they also hash equal; otherwise
results are nondeterminate, and we fail bootstrap comparison. */
- gcc_assert (gimple_tree_hash (p1) == gimple_tree_hash (p2));
+ gcc_assert (hash (p1) == hash (p2));
#endif
- return 1;
+ return true;
}
struct gimplify_ctx
--
Lawrence Crowl
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On 12/4/12, Diego Novillo wrote: > On Tue, Dec 4, 2012 at 4:23 AM, Richard Biener > wrote: >> On Mon, Dec 3, 2012 at 9:14 PM, Lawrence Crowl >> wrote: >>> On 12/3/12, Diego Novillo wrote: >>>> On 2012-12-01 20:44 , Lawrence Crowl wrote: >>>>> Index: gcc/gimple-fold.c >>>>> === >>>>> --- gcc/gimple-fold.c(revision 193902) >>>>> +++ gcc/gimple-fold.c(working copy) >>>>> @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. >>>>> #include "tree-ssa-propagate.h" >>>>> #include "target.h" >>>>> #include "gimple-fold.h" >>>>> +#include "gimplify-ctx.h" >>>>> >>>>> /* Return true when DECL can be referenced from current unit. >>>>> FROM_DECL (if non-null) specify constructor of variable DECL was >>>>> taken from. >>>>> Index: gcc/tree-mudflap.c >>>>> === >>>>> --- gcc/tree-mudflap.c (revision 193902) >>>>> +++ gcc/tree-mudflap.c (working copy) >>>>> @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. >>>>> #include "ggc.h" >>>>> #include "cgraph.h" >>>>> #include "gimple.h" >>>>> +#include "gimplify-ctx.h" >>>>> >>>>> extern void add_bb_to_loop (basic_block, struct loop *); >>>>> >>>>> Index: gcc/tree-inline.c >>>>> === >>>>> --- gcc/tree-inline.c(revision 193902) >>>>> +++ gcc/tree-inline.c(working copy) >>>>> @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. >>>>> #include "value-prof.h" >>>>> #include "tree-pass.h" >>>>> #include "target.h" >>>>> +#include "gimplify-ctx.h" >>>> >>>> I don't follow. It seems that factoring into gimplify-ctx.h does >>>> not actually buy much. The files using it are just including >>>> *another* file. Whereas previously, they were getting that >>>> content from gimple.h. >>>> >>>> Unless we can stop including gimple.h from these files, I don't >>>> see a lot of gain in this factoring. Am I missing something? >>> >>> There at least 70 files that include gimple.h, and only 5 that need >>> gimple-ctx.h. By splitting it out, at least 65 files will not need >>> to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, >>> the gimplify_hasher template struct, and may not need to include >>> hash-table.h. >>> >>> It's all about avoiding superfluous compilation in other files. >> >> But it's backward - gimple.h is the core file as it provides GIMPLE. >> The gimplifier and its context certainly requires to know about >> GIMPLE. >> >> Btw, if we still want to split it I'd rather have gimplify.h (and also >> put all exports from gimplify.c there, not only gimplify_ctx). Still >> I don't think it would buy us much. > > We talked offline about this with Lawrence. I was not too convinced > about adding this new header file, it seemed forced but I was > struggling with a better alternative. Perhaps gimplify.h is a better > way, but I'm not sure either. > > We decided that since it's in the branch, it may not matter much for > now. However, another thing occurred to me in the meantime: merges. > The new file will probably cause merge problems. If we are not > completely convinced that it's a good change, we are making our life > harder for no gain. > > Lawrence, you're going to hate me, but would you mind just leaving the > ctx structure in gimple.h for now? Sorry! I think it's going to be > easier to just leave it there for now until we come up with a good > split for the constructs in that file. Okay, I'll do that. -- Lawrence Crowl
Re: [cxx-conversion] LTO-related hash tables
On 12/3/12, Diego Novillo wrote: > On 2012-12-01 20:40 , Lawrence Crowl wrote: > > Change LTO-related hash tables from htab_t to hash_table: > > > > lto-streamer.h output_block::string_hash_table > > lto-streamer-in.c file_name_hash_table > > lto-streamer.c tree_htab > > > > The struct string_slot moves from data-streamer.h to > > lto-streamer.h to resolve compilation dependences. > > What compilation dependences? If it was fine before in > data-streamer.h why does it need to move to lto-streamer.h? > I'm missing that connection. Before the change, lto-streamer.h output_block::string_hash_table was an htab_t. The element type was opaque, i.e. implicit in the void* casting in the hash functions provided at the htab_create call site. With the change to hash_table, the output_block::string_hash_table declaration needed string_slot_hasher which in turn needed string_slot. But string_slot was defined in data-streamer.h after output_block in lto-streamer.h. So, something had to move. -- Lawrence Crowl
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On 12/3/12, Diego Novillo wrote: > On 2012-12-01 20:44 , Lawrence Crowl wrote: >> Index: gcc/gimple-fold.c >> === >> --- gcc/gimple-fold.c(revision 193902) >> +++ gcc/gimple-fold.c(working copy) >> @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. >> #include "tree-ssa-propagate.h" >> #include "target.h" >> #include "gimple-fold.h" >> +#include "gimplify-ctx.h" >> >> /* Return true when DECL can be referenced from current unit. >> FROM_DECL (if non-null) specify constructor of variable DECL was >> taken from. >> Index: gcc/tree-mudflap.c >> === >> --- gcc/tree-mudflap.c (revision 193902) >> +++ gcc/tree-mudflap.c (working copy) >> @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. >> #include "ggc.h" >> #include "cgraph.h" >> #include "gimple.h" >> +#include "gimplify-ctx.h" >> >> extern void add_bb_to_loop (basic_block, struct loop *); >> >> Index: gcc/tree-inline.c >> === >> --- gcc/tree-inline.c(revision 193902) >> +++ gcc/tree-inline.c(working copy) >> @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. >> #include "value-prof.h" >> #include "tree-pass.h" >> #include "target.h" >> +#include "gimplify-ctx.h" > > I don't follow. It seems that factoring into gimplify-ctx.h does > not actually buy much. The files using it are just including > *another* file. Whereas previously, they were getting that > content from gimple.h. > > Unless we can stop including gimple.h from these files, I don't > see a lot of gain in this factoring. Am I missing something? There at least 70 files that include gimple.h, and only 5 that need gimple-ctx.h. By splitting it out, at least 65 files will not need to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, the gimplify_hasher template struct, and may not need to include hash-table.h. It's all about avoiding superfluous compilation in other files. -- Lawrence Crowl
Re: [cxx-conversion] tree-related hash tables
On 12/3/12, Diego Novillo wrote: > On 2012-12-01 20:45 , Lawrence Crowl wrote: >> Index: gcc/tree-hasher.h >> === >> --- gcc/tree-hasher.h(revision 0) >> +++ gcc/tree-hasher.h(revision 0) >> @@ -0,0 +1,56 @@ >> +/* Data and Control Flow Analysis for Trees. > > This is the wrong description for this file. > >> + Copyright (C) 2001, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, >> 2011, >> + 2012 Free Software Foundation, Inc. >> + Contributed by Diego Novillo > > Only mention year 2012 and don't blame me for sins I never committed ;) The dreaded "copy and fail to fully edit". Fixed. > OK otherwise. Thanks. -- Lawrence Crowl
Re: [cxx-conversion] ggc-common hash tables
On 12/3/12, Diego Novillo wrote: > On 2012-12-01 20:46 , Lawrence Crowl wrote: >> Index: gcc/ChangeLog >> >> 2012-11-30 Lawrence Crowl >> >> * hash-table.h (class hash_table): >> Correct many methods with parameter types compare_type to the correct >> value_type. (Correct code was unlikely to notice the change.) >> (hash_table::elements_with_deleted) New. > > ':' after ')'. Done. >> @@ -1024,11 +1035,10 @@ cmp_statistic (const void *loc1, const v >> >> /* Collect array of the descriptors from hashtable. */ >> static struct loc_descriptor **loc_array; >> -static int >> -add_statistics (void **slot, void *b) >> +int >> +ggc_add_statistics (loc_descriptor **slot, int *n) > > Why remove 'static'? It is now a template argument, and cannot be file-local. > OK otherwise. Thanks. -- Lawrence Crowl
Re: [cxx-conversion] various hash tables, part 2/2
On 12/3/12, Diego Novillo wrote:
> On 2012-12-01 20:48 , Lawrence Crowl wrote:
>> +inline bool
>> +cselib_hasher::equal (const value_type *v, const compare_type *x_arg)
>> +{
>> + struct elt_loc_list *l;
>> + rtx x = CONST_CAST_RTX (x_arg);
>> + enum machine_mode mode = GET_MODE (x);
>> +
>> + gcc_assert (!CONST_SCALAR_INT_P (x) && GET_CODE (x) != CONST_FIXED);
>> +
>> + if (mode != GET_MODE (v->val_rtx))
>> +return 0;
>> +
>> + /* Unwrap X if necessary. */
>> + if (GET_CODE (x) == CONST
>> + && (CONST_SCALAR_INT_P (XEXP (x, 0))
>> + || GET_CODE (XEXP (x, 0)) == CONST_FIXED))
>> +x = XEXP (x, 0);
>> +
>> + /* We don't guarantee that distinct rtx's have different hash values,
>> + so we need to do a comparison. */
>> + for (l = v->locs; l; l = l->next)
>> +if (rtx_equal_for_cselib_1 (l->loc, x, find_slot_memmode))
>> + {
>> +promote_debug_loc (l);
>> +return 1;
>> + }
>> +
>> + return 0;
>
> s/1/true/
> s/0/false/
Done.
>> @@ -504,8 +557,10 @@ cselib_get_next_uid (void)
>> return next_uid;
>> }
>>
>> +#if 0
>> /* See the documentation of cselib_find_slot below. */
>> static enum machine_mode find_slot_memmode;
>> +#endif
>
> Remove?
Done.
>> +#if 0
>> /* The equality test for our hash table. The first argument ENTRY is a
>> table
>> element (i.e. a cselib_val), while the second arg X is an rtx. We
>> know
>> that all callers of htab_find_slot_with_hash will wrap CONST_INTs
>> into a
>> @@ -570,6 +626,7 @@ get_value_hash (const void *entry)
>> const cselib_val *const v = (const cselib_val *) entry;
>> return v->hash;
>> }
>> +#endif
>
> Remove?
Done.
> OK otherwise.
>
> This only leaves the GC htabs, right?
No, there are still more non-GC htabs.
> What was the issue there? gengtype being silly?
Yes.
--
Lawrence Crowl
Re: [cxx-conversion] various hash tables, part 1/2
On 12/3/12, Diego Novillo wrote: > On 2012-12-03 14:24 , Lawrence Crowl wrote: >>>> -static int >>>> -htab_cu_eq (const void *of1, const void *of2) >>>> +inline bool >>>> +cu_hash_table_entry_hasher::equal (const value_type *entry1, >>>> + const compare_type *entry2) >>> >>> No static? >> >> The in-class declaration has the static keyword. (It's still a >> global symbol though, not local.) > > Ah, right. I've seen other cases where the patch removes the > 'static' qualifier in free functions, though (in the second patch > too). Did you need to make those functions externally available? In C++03, any function argument to a template needs to be externally available. These functions are parameters to the traverse templates. -- Lawrence Crowl
Re: [cxx-conversion] various hash tables, part 1/2
On 12/3/12, Diego Novillo wrote:
> On Sat, Dec 1, 2012 at 8:47 PM, Lawrence Crowl wrote:
>
>> +inline bool
>> +attribute_hasher::equal (const value_type *spec, const compare_type
>> *str)
>> +{
>> + return (!strncmp (spec->name, str->str, str->length)
>
> I have a slight preference for strncmp() == 0. It's easier to read (I
> realize that you are just cut-n-pasting old code here :)
Done.
>> @@ -76,13 +82,11 @@ bitmap_descriptor (const char *file, int
>>loc.function = function;
>>loc.line = line;
>>
>> - if (!bitmap_desc_hash)
>> -bitmap_desc_hash = htab_create (10, hash_descriptor, eq_descriptor,
>> NULL);
>> + if (!bitmap_desc_hash.is_created ())
>> +bitmap_desc_hash.create (10);
>>
>> - slot = (struct bitmap_descriptor **)
>> -htab_find_slot_with_hash (bitmap_desc_hash, &loc,
>> - htab_hash_pointer (file) + line,
>> - INSERT);
>> + slot = bitmap_desc_hash
>> +.find_slot_with_hash (&loc, htab_hash_pointer (file) + line,
>> INSERT);
>
> I'd rather split the argument list here.
Done, though it becomes three lines instead of two.
>
>> }
>>
>> -static int
>> -htab_cu_eq (const void *of1, const void *of2)
>> +inline bool
>> +cu_hash_table_entry_hasher::equal (const value_type *entry1,
>> + const compare_type *entry2)
>
> No static?
The in-class declaration has the static keyword. (It's still a
global symbol though, not local.)
>> -static hashval_t
>> -ttypes_filter_hash (const void *pentry)
>> +inline hashval_t
>> +ttypes_filter_hasher::hash (const value_type *entry)
>> {
>> - const struct ttypes_filter *entry = (const struct ttypes_filter *)
>> pentry;
>>return TREE_HASH (entry->t);
>
> The patch seems to have changed the types here. Is it value_type or
> ttypes_filter?
The hasher class defines value_type as a typedef to ttypes_filter.
There has been no change in actual type.
> Looks OK otherwise.
Okay, I'll put it in the commit queue.
--
Lawrence Crowl
[cxx-conversion] ggc-common hash tables
Change ggc-common hash tables from htab_t to hash_table:
ggc-common.c loc_hash
ggc-common.c ptr_hash
Add a new hash_table method elements_with_deleted to meet the needs of
gcc-common.c.
Correct many methods with parameter types compare_type to the correct
value_type. (Correct code was unlikely to notice the change, but
incorrect code will.)
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl
* hash-table.h (class hash_table):
Correct many methods with parameter types compare_type to the correct
value_type. (Correct code was unlikely to notice the change.)
(hash_table::elements_with_deleted) New.
* ggc-common.c (htab_t saving_htab):
Change type to hash_table. Update dependent calls and types.
(htab_t loc_hash): Likewise.
(htab_t ptr_hash): Likewise.
(call_count): Rename ggc_call_count.
(call_alloc): Rename ggc_call_alloc.
(loc_descriptor): Rename make_loc_descriptor.
(add_statistics): Rename ggc_add_statistics.
* Makefile.in: Update to changes above.
Index: gcc/hash-table.h
===
--- gcc/hash-table.h(revision 193902)
+++ gcc/hash-table.h(working copy)
@@ -380,19 +380,19 @@ public:
void create (size_t initial_slots);
bool is_created ();
void dispose ();
- value_type *find (const compare_type *comparable);
+ value_type *find (const value_type *value);
value_type *find_with_hash (const compare_type *comparable, hashval_t hash);
- value_type **find_slot (const compare_type *comparable,
- enum insert_option insert);
+ value_type **find_slot (const value_type *value, enum insert_option insert);
value_type **find_slot_with_hash (const compare_type *comparable,
hashval_t hash, enum insert_option insert);
void empty ();
void clear_slot (value_type **slot);
- void remove_elt (const compare_type *comparable);
+ void remove_elt (const value_type *value);
void remove_elt_with_hash (const compare_type *comparable, hashval_t hash);
- size_t size();
- size_t elements();
- double collisions();
+ size_t size ();
+ size_t elements ();
+ size_t elements_with_deleted ();
+ double collisions ();
template
@@ -431,9 +431,9 @@ hash_table ::is_c
template class Allocator>
inline typename Descriptor::value_type *
-hash_table ::find (const compare_type *comparable)
+hash_table ::find (const value_type *value)
{
- return find_with_hash (comparable, Descriptor::hash (comparable));
+ return find_with_hash (value, Descriptor::hash (value));
}
@@ -443,9 +443,9 @@ template class Allocator>
inline typename Descriptor::value_type **
hash_table
-::find_slot (const compare_type *comparable, enum insert_option insert)
+::find_slot (const value_type *value, enum insert_option insert)
{
- return find_slot_with_hash (comparable, Descriptor::hash
(comparable), insert);
+ return find_slot_with_hash (value, Descriptor::hash (value), insert);
}
@@ -454,9 +454,9 @@ hash_table
template class Allocator>
inline void
-hash_table ::remove_elt (const compare_type *comparable)
+hash_table ::remove_elt (const value_type *value)
{
- remove_elt_with_hash (comparable, Descriptor::hash (comparable));
+ remove_elt_with_hash (value, Descriptor::hash (value));
}
@@ -476,12 +476,23 @@ hash_table ::size
template class Allocator>
inline size_t
-hash_table ::elements()
+hash_table ::elements ()
{
return htab->n_elements - htab->n_deleted;
}
+/* Return the current number of elements in this hash table. */
+
+template class Allocator>
+inline size_t
+hash_table ::elements_with_deleted ()
+{
+ return htab->n_elements;
+}
+
+
/* Return the fraction of fixed collisions during all work with given
hash table. */
Index: gcc/ggc-common.c
===
--- gcc/ggc-common.c(revision 193902)
+++ gcc/ggc-common.c(working copy)
@@ -24,7 +24,7 @@ along with GCC; see the file COPYING3.
#include "config.h"
#include "system.h"
#include "coretypes.h"
-#include "hashtab.h"
+#include "hash-table.h"
#include "ggc.h"
#include "ggc-internal.h"
#include "diagnostic-core.h"
@@ -47,10 +47,6 @@ static ggc_statistics *ggc_stats;
struct traversal_state;
static int ggc_htab_delete (void **, void *);
-static hashval_t saving_htab_hash (const void *);
-static int saving_htab_eq (const void *, const void *);
-static int call_count (void **, void *);
-static int call_alloc (void **, void *);
static int compare_ptr_data (const void *, const void *);
static void relocate_ptrs (void *, void *);
static void write_pch_globals (const struct ggc_root_tab * const *tab,
@@ -291,8 +287,6 @@ ggc_print_common_statistics (FILE *strea
/* Functions for saving and restori
[cxx-conversion] tree-related hash tables
Change tree-related hash tables from htab_t to hash_table:
tree-complex.c complex_variable_components
tree-parloops.c eliminate_local_variables_stmt::decl_address
tree-parloops.c separate_decls_in_region::decl_copies
Move hash table declarations to a new tree-hasher.h, to resolve
compilation dependences and because they are used in few places.
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl
* tree-hasher.h: New.
(struct int_tree_hasher): New.
(typedef int_tree_htab_type): New.
* tree-flow.h (extern int_tree_map_hash): Moved into tree-hasher
struct int_tree_hasher.
(extern int_tree_map_eq): Likewise.
* tree-complex.c: Include tree-hasher.h
(htab_t complex_variable_components):
Change type to hash_table. Update dependent calls and types.
* tree-parloops.c: Include tree-hasher.h.
(htab_t eliminate_local_variables_stmt::decl_address):
Change type to hash_table. Update dependent calls and types.
(htab_t separate_decls_in_region::decl_copies): Likewise.
* tree-ssa.c (int_tree_map_eq): Moved into struct int_tree_hasher
in tree-flow.h.
(int_tree_map_hash): Likewise.
* Makefile.in: Update to changes above.
Index: gcc/tree-complex.c
===
--- gcc/tree-complex.c (revision 193902)
+++ gcc/tree-complex.c (working copy)
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.
#include "tree-iterator.h"
#include "tree-pass.h"
#include "tree-ssa-propagate.h"
+#include "tree-hasher.h"
/* For each complex ssa name, a lattice value. We're interested in finding
@@ -54,7 +55,7 @@ static vec complex_la
/* For each complex variable, a pair of variables for the components exists in
the hashtable. */
-static htab_t complex_variable_components;
+static int_tree_htab_type complex_variable_components;
/* For each complex SSA_NAME, a pair of ssa names for the components. */
static vec complex_ssa_name_components;
@@ -66,7 +67,7 @@ cvc_lookup (unsigned int uid)
{
struct int_tree_map *h, in;
in.uid = uid;
- h = (struct int_tree_map *) htab_find_with_hash
(complex_variable_components, &in, uid);
+ h = complex_variable_components.find_with_hash (&in, uid);
return h ? h->to : NULL;
}
@@ -76,14 +77,13 @@ static void
cvc_insert (unsigned int uid, tree to)
{
struct int_tree_map *h;
- void **loc;
+ int_tree_map **loc;
h = XNEW (struct int_tree_map);
h->uid = uid;
h->to = to;
- loc = htab_find_slot_with_hash (complex_variable_components, h,
- uid, INSERT);
- *(struct int_tree_map **) loc = h;
+ loc = complex_variable_components.find_slot_with_hash (h, uid, INSERT);
+ *loc = h;
}
/* Return true if T is not a zero constant. In the case of real values,
@@ -1569,8 +1569,7 @@ tree_lower_complex (void)
init_parameter_lattice_values ();
ssa_propagate (complex_visit_stmt, complex_visit_phi);
- complex_variable_components = htab_create (10, int_tree_map_hash,
-int_tree_map_eq, free);
+ complex_variable_components.create (10);
complex_ssa_name_components.create (2 * num_ssa_names);
complex_ssa_name_components.safe_grow_cleared (2 * num_ssa_names);
@@ -1591,7 +1590,7 @@ tree_lower_complex (void)
gsi_commit_edge_inserts ();
- htab_delete (complex_variable_components);
+ complex_variable_components.dispose ();
complex_ssa_name_components.release ();
complex_lattice_values.release ();
return 0;
Index: gcc/tree-hasher.h
===
--- gcc/tree-hasher.h (revision 0)
+++ gcc/tree-hasher.h (revision 0)
@@ -0,0 +1,56 @@
+/* Data and Control Flow Analysis for Trees.
+ Copyright (C) 2001, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011,
+ 2012 Free Software Foundation, Inc.
+ Contributed by Diego Novillo
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#ifndef GCC_TREE_HASHER_H
+#define GCC_TREE_HASHER_H 1
+
+#include "hash-table.h"
+#include "tree-flow.h"
+
+/* Hashtable helpers. */
+
+struct int_tree_hasher : typed_free_remove
+{
+ typedef int_tree_map value_type;
+ typedef int_tree_map compare_type;
[cxx-conversion] gimplify_ctx::temp_htab hash table
Change gimplify.c gimplify_ctx::temp_htab hash table from htab_t to
hash_table.
Move struct gimple_temp_hash_elt and struct gimplify_ctx to a new
gimplify-ctx.h, because they are used few places.
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl
* gimple.h (struct gimplify_ctx): Move to gimplify-ctx.h.
(push_gimplify_context): Likewise.
(pop_gimplify_context): Likewise.
* gimplify-ctx.h: New.
(struct gimple_temp_hash_elt): Move from gimplify.c.
(class gimplify_hasher): New.
(struct gimplify_ctx): Move from gimple.h.
(htab_t gimplify_ctx::temp_htab):
Change type to hash_table. Update dependent calls and types.
* gimple-fold.c: Include gimplify-ctx.h.
* gimplify.c: Include gimplify-ctx.h.
(struct gimple_temp_hash_elt): Move to gimplify-ctx.h.
(htab_t gimplify_ctx::temp_htab):
Update dependent calls and types for new type hash_table.
(gimple_tree_hash): Move into gimplify_hasher in gimplify-ctx.h.
(gimple_tree_eq): Move into gimplify_hasher in gimplify-ctx.h.
* omp-low.c: Include gimplify-ctx.h.
* tree-inline.c: Include gimplify-ctx.h.
* tree-mudflap.c: Include gimplify-ctx.h.
* Makefile.in: Update to changes above.
Index: gcc/omp-low.c
===
--- gcc/omp-low.c (revision 193902)
+++ gcc/omp-low.c (working copy)
@@ -29,6 +29,7 @@ along with GCC; see the file COPYING3.
#include "tree.h"
#include "rtl.h"
#include "gimple.h"
+#include "gimplify-ctx.h"
#include "tree-iterator.h"
#include "tree-inline.h"
#include "langhooks.h"
Index: gcc/gimplify.c
===
--- gcc/gimplify.c (revision 193902)
+++ gcc/gimplify.c (working copy)
@@ -44,6 +44,7 @@ along with GCC; see the file COPYING3.
#include "splay-tree.h"
#include "vec.h"
#include "gimple.h"
+#include "gimplify-ctx.h"
#include "langhooks-def.h" /* FIXME: for lhd_set_decl_assembler_name */
#include "tree-pass.h" /* FIXME: only for PROP_gimple_any */
@@ -88,15 +89,6 @@ static struct gimplify_ctx *gimplify_ctx
static struct gimplify_omp_ctx *gimplify_omp_ctxp;
-/* Formal (expression) temporary table handling: multiple occurrences of
- the same scalar expression are evaluated into the same temporary. */
-
-typedef struct gimple_temp_hash_elt
-{
- tree val; /* Key */
- tree temp; /* Value */
-} elt_t;
-
/* Forward declaration. */
static enum gimplify_status gimplify_compound_expr (tree *,
gimple_seq *, bool);
@@ -131,40 +123,6 @@ mark_addressable (tree x)
}
}
-/* Return a hash value for a formal temporary table entry. */
-
-static hashval_t
-gimple_tree_hash (const void *p)
-{
- tree t = ((const elt_t *) p)->val;
- return iterative_hash_expr (t, 0);
-}
-
-/* Compare two formal temporary table entries. */
-
-static int
-gimple_tree_eq (const void *p1, const void *p2)
-{
- tree t1 = ((const elt_t *) p1)->val;
- tree t2 = ((const elt_t *) p2)->val;
- enum tree_code code = TREE_CODE (t1);
-
- if (TREE_CODE (t2) != code
- || TREE_TYPE (t1) != TREE_TYPE (t2))
-return 0;
-
- if (!operand_equal_p (t1, t2, 0))
-return 0;
-
-#ifdef ENABLE_CHECKING
- /* Only allow them to compare equal if they also hash equal; otherwise
- results are nondeterminate, and we fail bootstrap comparison. */
- gcc_assert (gimple_tree_hash (p1) == gimple_tree_hash (p2));
-#endif
-
- return 1;
-}
-
/* Link gimple statement GS to the end of the sequence *SEQ_P. If
*SEQ_P is NULL, a new sequence is allocated. This function is
similar to gimple_seq_add_stmt, but does not scan the operands.
@@ -242,8 +200,8 @@ pop_gimplify_context (gimple body)
else
record_vars (c->temps);
- if (c->temp_htab)
-htab_delete (c->temp_htab);
+ if (c->temp_htab.is_created ())
+c->temp_htab.dispose ();
}
/* Push a GIMPLE_BIND tuple onto the stack of bindings. */
@@ -586,23 +544,22 @@ lookup_tmp_var (tree val, bool is_formal
else
{
elt_t elt, *elt_p;
- void **slot;
+ elt_t **slot;
elt.val = val;
- if (gimplify_ctxp->temp_htab == NULL)
-gimplify_ctxp->temp_htab
- = htab_create (1000, gimple_tree_hash, gimple_tree_eq, free);
- slot = htab_find_slot (gimplify_ctxp->temp_htab, (void *)&elt, INSERT);
+ if (!gimplify_ctxp->temp_htab.is_created ())
+gimplify_ctxp->temp_htab.create (1000);
+ slot = gimplify_ctxp->temp_htab.find_slot (&elt, INSERT);
if (*slot == NULL)
{
elt_p = XNEW (elt_t);
elt_p->val = val;
elt_p->temp = ret = create_tmp_from_val (val, is_fo
[cxx-conversion] graphite-related hash tables
Change graphite-related hash tables from htab_t to hash_table:
graphite-clast-to-gimple.c ivs_params::newivs_index
graphite-clast-to-gimple.c ivs_params::params_index
graphite-clast-to-gimple.c print_generated_program::params_index
graphite-clast-to-gimple.c gloog::newivs_index
graphite-clast-to-gimple.c gloog::params_index
graphite.c graphite_transform_loops::bb_pbb_mapping
sese.c copy_bb_and_scalar_dependences::rename_map
Move hash table declarations to a new graphite-htab.h, because they
are used in few places.
Remove unused:
htab_t scop::original_pddrs
SCOP_ORIGINAL_PDDRS
Remove unused:
insert_loop_close_phis
insert_guard_phis
debug_ivtype_map
ivtype_map_elt_info
new_ivtype_map_elt
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl
* graphite-htab.h: New.
(typedef hash_table bb_pbb_htab_type): New.
(extern find_pbb_via_hash): Move from graphite-poly.h.
(extern loop_is_parallel_p): Move from graphite-poly.h.
(extern get_loop_body_pbbs): Move from graphite-poly.h.
* graphite-clast-to-gimple.h: (extern gloog) Move to graphite-htab.h.
(bb_pbb_map_hash): Fold into bb_pbb_htab_type in graphite-htab.h.
(eq_bb_pbb_map): Fold into bb_pbb_htab_type in graphite-htab.h.
* graphite-clast-to-gimple.c: Include graphite-htab.h.
(htab_t ivs_params::newivs_index):
Change type to hash_table. Update dependent calls and types.
(htab_t ivs_params::params_index): Likewise.
(htab_t print_generated_program::params_index): Likewise.
(htab_t gloog::newivs_index): Likewise.
(htab_t gloog::params_index): Likewise.
* graphite-dependences.c: Include graphite-htab.h.
(loop_is_parallel_p): Change hash table type of parameter.
* graphite-poly.h (htab_t scop::original_pddrs): Remove unused.
(SCOP_ORIGINAL_PDDRS): Remove unused.
(extern find_pbb_via_hash): Move to graphite-htab.h.
(extern loop_is_parallel_p): Move to graphite-htab.h.
(extern get_loop_body_pbbs): Move to graphite-htab.h.
* graphite.c: Include graphite-htab.h.
(htab_t graphite_transform_loops::bb_pbb_mapping):
Change type to hash_table. Update dependent calls and types.
* sese.h (extern insert_loop_close_phis): Remove unused.
(extern insert_guard_phis): Remove unused.
(extern debug_ivtype_map): Remove unused.
(extern ivtype_map_elt_info): Remove unused.
(inline new_ivtype_map_elt): Remove unused.
(extern debug_rename_map): Move to .c file.
* sese.c (debug_rename_map_1): Make extern.
(debug_ivtype_elt): Remove unused.
(debug_ivtype_map_1): Remove unused.
(debug_ivtype_map): Remove unused.
(ivtype_map_elt_info): Remove unused.
(eq_ivtype_map_elts): Remove unused.
(htab_t copy_bb_and_scalar_dependences::rename_map):
Change type to hash_table. Update dependent calls and types.
* Makefile.in: Update to changes above.
Index: gcc/graphite-htab.h
===
--- gcc/graphite-htab.h (revision 0)
+++ gcc/graphite-htab.h (revision 0)
@@ -0,0 +1,60 @@
+/* Translation of CLAST (CLooG AST) to Gimple.
+ Copyright (C) 2012 Free Software Foundation, Inc.
+ Contributed by Sebastian Pop .
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#ifndef GCC_GRAPHITE_HTAB_H
+#define GCC_GRAPHITE_HTAB_H
+
+#include "hash-table.h"
+#include "graphite-clast-to-gimple.h"
+
+/* Hashtable helpers. */
+
+struct bb_pbb_hasher : typed_free_remove
+{
+ typedef bb_pbb_def value_type;
+ typedef bb_pbb_def compare_type;
+ static inline hashval_t hash (const value_type *);
+ static inline bool equal (const value_type *, const compare_type *);
+};
+
+/* Hash function for data base element BB_PBB. */
+
+inline hashval_t
+bb_pbb_hasher::hash (const value_type *bb_pbb)
+{
+ return (hashval_t)(bb_pbb->bb->index);
+}
+
+/* Compare data base element PB1 and PB2. */
+
+inline bool
+bb_pbb_hasher::equal (const value_type *bp1, const compare_type *bp2)
+{
+ return (bp1->bb->index == bp2->bb->index);
+}
+
+typedef hash_table bb_pbb_htab_type;
+
+extern bool gloog (scop_p, bb_pbb_htab_type);
+poly_bb_p find_pbb_via_hash (bb_pbb_htab_type, ba
[cxx-conversion] LTO-related hash tables
Change LTO-related hash tables from htab_t to hash_table:
lto-streamer.h output_block::string_hash_table
lto-streamer-in.c file_name_hash_table
lto-streamer.c tree_htab
The struct string_slot moves from data-streamer.h to lto-streamer.h to
resolve compilation dependences.
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl
* data-streamer.h (struct string_slot): Move to lto-streamer.h.
(hash_string_slot_node): Move implementation into lto-streamer.h
struct string_slot_hasher.
(eq_string_slot_node): Likewise.
* data-streamer-out.c: Update output_block::string_hash_table
dependent calls and types.
* lto-streamer.h (struct string_slot): Move from data-streamer.h
(struct string_slot_hasher): New.
(htab_t output_block::string_hash_table):
Change type to hash_table. Update dependent calls and types.
* lto-streamer-in.c (freeing_string_slot_hasher): New.
(htab_t file_name_hash_table):
Change type to hash_table. Update dependent calls and types.
* lto-streamer-out.c: Update output_block::string_hash_table dependent
calls and types.
* lto-streamer.c (htab_t tree_htab):
Change type to hash_table. Update dependent calls and types.
* Makefile.in: Update to changes above.
Index: gcc/data-streamer-out.c
===
--- gcc/data-streamer-out.c (revision 193902)
+++ gcc/data-streamer-out.c (working copy)
@@ -42,8 +42,7 @@ streamer_string_index (struct output_blo
s_slot.len = len;
s_slot.slot_num = 0;
- slot = (struct string_slot **) htab_find_slot (ob->string_hash_table,
-&s_slot, INSERT);
+ slot = ob->string_hash_table.find_slot (&s_slot, INSERT);
if (*slot == NULL)
{
struct lto_output_stream *string_stream = ob->string_stream;
Index: gcc/lto-streamer-out.c
===
--- gcc/lto-streamer-out.c (revision 193902)
+++ gcc/lto-streamer-out.c (working copy)
@@ -77,8 +77,7 @@ create_output_block (enum lto_section_ty
clear_line_info (ob);
- ob->string_hash_table = htab_create (37, hash_string_slot_node,
- eq_string_slot_node, NULL);
+ ob->string_hash_table.create (37);
gcc_obstack_init (&ob->obstack);
return ob;
@@ -92,7 +91,7 @@ destroy_output_block (struct output_bloc
{
enum lto_section_type section_type = ob->section_type;
- htab_delete (ob->string_hash_table);
+ ob->string_hash_table.dispose ();
free (ob->main_stream);
free (ob->string_stream);
Index: gcc/lto-streamer-in.c
===
--- gcc/lto-streamer-in.c (revision 193902)
+++ gcc/lto-streamer-in.c (working copy)
@@ -49,8 +49,19 @@ along with GCC; see the file COPYING3.
#include "tree-pass.h"
#include "streamer-hooks.h"
+struct freeing_string_slot_hasher : string_slot_hasher
+{
+ static inline void remove (value_type *);
+};
+
+inline void
+freeing_string_slot_hasher::remove (value_type *v)
+{
+ free (v);
+}
+
/* The table to hold the file names. */
-static htab_t file_name_hash_table;
+static hash_table file_name_hash_table;
/* Check that tag ACTUAL has one of the given values. NUM_TAGS is the
@@ -94,14 +105,14 @@ lto_input_data_block (struct lto_input_b
static const char *
canon_file_name (const char *string)
{
- void **slot;
+ string_slot **slot;
struct string_slot s_slot;
size_t len = strlen (string);
s_slot.s = string;
s_slot.len = len;
- slot = htab_find_slot (file_name_hash_table, &s_slot, INSERT);
+ slot = file_name_hash_table.find_slot (&s_slot, INSERT);
if (*slot == NULL)
{
char *saved_string;
@@ -117,7 +128,7 @@ canon_file_name (const char *string)
}
else
{
- struct string_slot *old_slot = (struct string_slot *) *slot;
+ struct string_slot *old_slot = *slot;
return old_slot->s;
}
}
@@ -1150,8 +1161,7 @@ void
lto_reader_init (void)
{
lto_streamer_init ();
- file_name_hash_table = htab_create (37, hash_string_slot_node,
- eq_string_slot_node, free);
+ file_name_hash_table.create (37);
}
Index: gcc/data-streamer.h
===
--- gcc/data-streamer.h (revision 193902)
+++ gcc/data-streamer.h (working copy)
@@ -44,15 +44,6 @@ struct bitpack_d
void *stream;
};
-
-/* String hashing. */
-struct string_slot
-{
- const char *s;
- int len;
- unsigned int slot_num;
-};
-
/* In data-streamer.c */
void bp_pack_var_len_unsigned (struct bitpack_d *, unsigned HOST_WIDE_INT);
void bp_pack_var_len_int (struct bitpack_d *, HOST_WIDE_INT);
@@ -90,35 +81,6 @@ const char *bp_
Re: [patch] Contribute performance comparison script.
On 11/6/12, Diego Novillo wrote: > On Nov 5, 2012 Lawrence Crowl wrote: > > > > 2012-11-05 Lawrence Crowl > > > > * compare_two_ftime_report_sets: New. > > OK. Thanks. Committed. -- Lawrence Crowl
[patch] Contribute performance comparison script.
Add a contrib script for comparing the performance of two sets of compiler runs. Usage documentation is in the script. The script produces output of the form: $ compare_two_ftime_report_sets "Log0/*perf" "Log3/*perf" Arithmetic sample for timevar log files "Log0/*perf" and selecting lines containing "TOTAL" with desired confidence 95 is trial count is 4, mean is 443.022 (95% confidence in 440.234 to 445.811), std.deviation is 1.75264, std.error is 0.876322 Arithmetic sample for timevar log files "Log3/*perf" and selecting lines containing "TOTAL" with desired confidence 95 is trial count is 4, mean is 441.302 (95% confidence in 436.671 to 445.934), std.deviation is 2.91098, std.error is 1.45549 The first sample appears to be 0.39% larger, with 60% confidence of being larger. To reach 95% confidence, you need roughly 14 trials, assuming the standard deviation is stable, which is iffy. Tested on x86_64 builds. Okay for trunk? Index: contrib/ChangeLog 2012-11-05 Lawrence Crowl * compare_two_ftime_report_sets: New. Index: contrib/compare_two_ftime_report_sets === --- contrib/compare_two_ftime_report_sets (revision 0) +++ contrib/compare_two_ftime_report_sets (revision 0) @@ -0,0 +1,605 @@ +#!/usr/bin/python + +# Script to statistically compare two sets of log files with -ftime-report +# output embedded within them. + +# Contributed by Lawrence Crowl +# +# Copyright (C) 2012 Free Software Foundation, Inc. +# +# This file is part of GCC. +# +# GCC is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3, or (at your option) +# any later version. +# +# GCC is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with GCC; see the file COPYING. If not, write to +# the Free Software Foundation, 51 Franklin Street, Fifth Floor, +# Boston, MA 02110-1301, USA. + + +""" Compare two sets of compile-time performance numbers. + +The intent of this script is to compare compile-time performance of two +different versions of the compiler. Each version of the compiler must be +run at least three times with the -ftime-report option. Each log file +represents a data point, or trial. The set of trials for each compiler +version constitutes a sample. The ouput of the script is a description +of the statistically significant difference between the two version of +the compiler. + +The parameters to the script are: + + Two file patterns that each match a set of log files. You will probably + need to quote the patterns before passing them to the script. + +Each pattern corresponds to a version of the compiler. + + A regular expression that finds interesting lines in the log files. + If you want to match the beginning of the line, you will need to add + the ^ operator. The filtering uses Python regular expression syntax. + +The default is "TOTAL". + +All of the interesting lines in a single log file are summed to produce +a single trial (data point). + + A desired statistical confidence within the range 60% to 99.9%. Due to + the implementation, this confidence will be rounded down to one of 60%, + 70%, 80%, 90%, 95%, 98%, 99%, 99.5%, 99.8%, and 99.9%. + +The default is 95. + +If the computed confidence is lower than desired, the script will +estimate the number of trials needed to meet the desired confidence. +This estimate is not very good, as the variance tends to change as +you increase the number of trials. + +The most common use of the script is total compile-time comparison between +logfiles stored in different directories. + +compare_two_ftime_report_sets "Log1/*perf" "Log2/*perf" + +One can also look at parsing time, but expecting a lower confidence. + +compare_two_ftime_report_sets "Log1/*perf" "Log2/*perf" "^phase parsing" 75 + +""" + + +import os +import sys +import fnmatch +import glob +import re +import math + + +### Utility + + +def divide(dividend, divisor): + """ Return the quotient, avoiding division by zero. + """ + if divisor == 0: +return sys.float_info.max + else: +return dividend / divisor + + +# File and Line + + +# Should you repurpose this script, this code might help. +# +#def find_files(topdir, filepat): +# """ Find a set of file names, under a given di
Re: [patch] Apply conditional down cast to cgraph.h et.al.
On 11/2/12, Eric Botcazou wrote:
>> Index: gcc/ChangeLog
>>
>> 2012-10-31 Lawrence Crowl
>>
>> * is-a.h: New.
>> (is_a (U*)): New. Test for is-a relationship.
>> (as_a (U*)): New. Treat as a derived type.
>> (dyn_cast (U*)): New. Conditionally cast based on is_a.
>> * cgraph.h (varpool_node): Rename to varpool_node_for_decl.
>> Adjust callers to match.
>> (is_a_helper ::test (symtab_node_def *)): New.
>> (is_a_helper ::test (symtab_node_def *)): New.
>> (symtab_node_def::try_function): New. Change most calls to
>> symtab_function_p with calls to dyn_cast (p).
>> (symtab_node_def::try_variable): New. Change most calls to
>> symtab_variable_p with calls to dyn_cast (p).
>> (symtab_function_p): Remove. Change callers to use
>> is_a (p) instead.
>> (symtab_variable_p): Remove. Change callers to use
>> is_a (p) instead.
>> * cgraph.c (cgraph_node_for_asm): Remove redundant call to
>> symtab_node_for_asm.
>> * cgraphunit.c (symbol_finalized_and_needed): New.
>> (symbol_finalized): New.
>> (cgraph_analyze_functions): Split complicated conditionals out into
>> above new functions.
>> * Makefile.in (CGRAPH_H): Add is-a.h as used by cgraph.h.
>
> The installed patch touches the ada/, cp/ and lto/ subdirectories without
> modifying their ChangeLog files. Please add the missing entries.
>
> [Some people, like me, do use these ChangeLogs to synchronize trees]
Done and committed.
Update ChangeLog files as requested for cgraph change to checked down cast.
Index: gcc/cp/ChangeLog
2012-10-31 Lawrence Crowl
* decl2.c (var_finalized_p): Rename varpool_node to
varpool_node_for_decl.
(maybe_emit_vtables): Likewise.
Index: gcc/ada/ChangeLog
2012-10-31 Lawrence Crowl
* gcc-interface/utils.c (gnat_write_global_declarations):
Rename varpool_node to varpool_node_for_decl.
Index: gcc/lto/ChangeLog
2012-10-31 Lawrence Crowl
* lto.c (lto_wpa_write_files): Change symtab checking to a checked
down-cast via dyn_cast.
* lto-partition.c (add_symbol_to_partition_1): Likewise.
(undo_partition): Likewise.
(lto_balanced_map): Likewise.
(get_symbol_class): Likewise and via is_a.
(lto_balanced_map): Change symtab checking to is_a.
--
Lawrence Crowl
Re: [patch] Remove unused ebitmap and unused sbitmap functions.
On 11/1/12, Diego Novillo wrote: > On 2012-10-31 18:47 , Lawrence Crowl wrote: >> 2012-10-31 Lawrence Crowl >> >> * ebitmap.h: Remove unused. >> * ebitmap.c: Remove unused. >> * Makefile.in: Remove ebitmap.h and ebitmap.c. >> * sbitmap.h (SBITMAP_SIZE_BYTES): Move to source file. >> (SET_BIT_WITH_POPCOUNT): Remove unused. >> (RESET_BIT_WITH_POPCOUNT): Remove unused. >> (bitmap_copy_n): Remove unused. >> (bitmap_range_empty_p): Remove unused. >> (sbitmap_popcount): Remove unused. >> (sbitmap_verify_popcount): Make private to source file. >> * sbitmap.c (SBITMAP_SIZE_BYTES): Move here from header. >> (bitmap_copy_n): Remove unused. >> (bitmap_range_empty_p): Remove unused. >> (sbitmap_popcount): Remove unused. >> (sbitmap_verify_popcount): Make private to source file. >> >> Index: gcc/sbitmap.c >> === >> --- gcc/sbitmap.c(revision 193049) >> +++ gcc/sbitmap.c(working copy) >> @@ -22,6 +22,9 @@ along with GCC; see the file COPYING3. >> #include "coretypes.h" >> #include "sbitmap.h" >> >> +/* Return the set size needed for N elements. */ >> +#define SBITMAP_SIZE_BYTES(BITMAP) ((BITMAP)->size * sizeof >> (SBITMAP_ELT_TYPE)) >> + > > Not terribly important, but maybe take advantage of the change and > convert it into a static inline function? Sure. > OK, otherwise. I will merge and commit. -- Lawrence Crowl
Re: [patch] Normalize bitmap iteration.
On 11/1/12, Diego Novillo wrote: > On 2012-10-31 13:43 , Lawrence Crowl wrote: > > Rename the EXECUTE_IF_SET_IN_SBITMAP macro to > > EXECUTE_IF_SET_IN_BITMAP. Its implementation is now identical to > > that in bitmap.h. To prevent redefinition errors, both definitions > > are now guarded by #ifndef. An alternate strategy is to simply > > include bitmap.h from sbitmap.h. As this would increase build time, > > I have elected to use the #ifndef version. I do not have a strong > > preference here. > > Me neither. This seems easy enough. > > > static inline void > > -sbitmap_iter_init (sbitmap_iterator *i, const_sbitmap bmp, unsigned int > > min) > > +bmp_iter_set_init (sbitmap_iterator *i, const_sbitmap bmp, > > + unsigned int min, unsigned *bit_no ATTRIBUTE_UNUSED) > > So, we'll be changing this again, right? Once we introduce the various > iterator types? If you mean C++ style iterators, yes. If you mean the 'computing' bitmap iterators, no. >> Index: gcc/bitmap.h >> === >> --- gcc/bitmap.h (revision 193006) >> +++ gcc/bitmap.h (working copy) >> @@ -682,10 +682,13 @@ bmp_iter_and_compl (bitmap_iterator *bi, >> should be treated as a read-only variable as it contains loop >> state. */ >> >> +#ifndef EXECUTE_IF_SET_IN_BITMAP >> +/* See sbitmap.h for the other definition of EXECUTE_IF_SET_IN_BITMAP. >> */ > > Ah... if we could only overload macro defintions ;) I had the same though. > The patch is OK. Thanks. Okay, I'll merge and commit. -- Lawrence Crowl
Re: [patch] Normalize more bitmap function names.
On 11/1/12, Diego Novillo wrote: > On 2012-10-31 13:41 , Lawrence Crowl wrote: >> This patch normalizes more bitmap function names. >> >>sbitmap.h >> >> TEST_BIT -> bitmap_bit_p >> SET_BIT -> bitmap_set_bit > > I wonder if it wouldn't it be more consistent if TEST_BIT became > bitmap_test_bit() ? I never particularly liked the lispy *_p > convention. But I don't really care much one way or the other. The plan was to simply use the bitmap names, and given the schedule I would prefer to leave it as is. > The patch is OK. Okay. I will merge and commit. -- Lawrence Crowl
[patch] Remove unused ebitmap and unused sbitmap functions.
This patch removes the unused ebitmap, and then removes some sbitmap functions
only used by ebitmap. The functions removed are:
SET_BIT_WITH_POPCOUNT
RESET_BIT_WITH_POPCOUNT
bitmap_copy_n
bitmap_range_empty_p
sbitmap_popcount
In addition, two functions have been made private to the implementation file:
SBITMAP_SIZE_BYTES
sbitmap_verify_popcount
Tested on x86-64.
Okay for trunk?
Index: gcc/ChangeLog
2012-10-31 Lawrence Crowl
* ebitmap.h: Remove unused.
* ebitmap.c: Remove unused.
* Makefile.in: Remove ebitmap.h and ebitmap.c.
* sbitmap.h (SBITMAP_SIZE_BYTES): Move to source file.
(SET_BIT_WITH_POPCOUNT): Remove unused.
(RESET_BIT_WITH_POPCOUNT): Remove unused.
(bitmap_copy_n): Remove unused.
(bitmap_range_empty_p): Remove unused.
(sbitmap_popcount): Remove unused.
(sbitmap_verify_popcount): Make private to source file.
* sbitmap.c (SBITMAP_SIZE_BYTES): Move here from header.
(bitmap_copy_n): Remove unused.
(bitmap_range_empty_p): Remove unused.
(sbitmap_popcount): Remove unused.
(sbitmap_verify_popcount): Make private to source file.
Index: gcc/sbitmap.c
===
--- gcc/sbitmap.c (revision 193049)
+++ gcc/sbitmap.c (working copy)
@@ -22,6 +22,9 @@ along with GCC; see the file COPYING3.
#include "coretypes.h"
#include "sbitmap.h"
+/* Return the set size needed for N elements. */
+#define SBITMAP_SIZE_BYTES(BITMAP) ((BITMAP)->size * sizeof (SBITMAP_ELT_TYPE))
+
/* This suffices for roughly 99% of the hosts we run on, and the rest
don't have 256 bit integers. */
#if SBITMAP_ELT_BITS > 255
@@ -53,7 +56,7 @@ typedef const SBITMAP_ELT_TYPE *const_sb
/* Verify the population count of sbitmap A matches the cached value,
if there is a cached value. */
-void
+static void
sbitmap_verify_popcount (const_sbitmap a)
{
unsigned ix;
@@ -245,16 +248,6 @@ bitmap_copy (sbitmap dst, const_sbitmap
memcpy (dst->popcount, src->popcount, sizeof (unsigned char) * dst->size);
}
-/* Copy the first N elements of sbitmap SRC to DST. */
-
-void
-bitmap_copy_n (sbitmap dst, const_sbitmap src, unsigned int n)
-{
- memcpy (dst->elms, src->elms, sizeof (SBITMAP_ELT_TYPE) * n);
- if (dst->popcount)
-memcpy (dst->popcount, src->popcount, sizeof (unsigned char) * n);
-}
-
/* Determine if a == b. */
int
bitmap_equal_p (const_sbitmap a, const_sbitmap b)
@@ -275,56 +268,6 @@ bitmap_empty_p (const_sbitmap bmap)
return true;
}
-/* Return false if any of the N bits are set in MAP starting at
- START. */
-
-bool
-bitmap_range_empty_p (const_sbitmap bmap, unsigned int start, unsigned int n)
-{
- unsigned int i = start / SBITMAP_ELT_BITS;
- SBITMAP_ELT_TYPE elm;
- unsigned int shift = start % SBITMAP_ELT_BITS;
-
- gcc_assert (bmap->n_bits >= start + n);
-
- elm = bmap->elms[i];
- elm = elm >> shift;
-
- if (shift + n <= SBITMAP_ELT_BITS)
-{
- /* The bits are totally contained in a single element. */
- if (shift + n < SBITMAP_ELT_BITS)
-elm &= ((1 << n) - 1);
- return (elm == 0);
-}
-
- if (elm)
-return false;
-
- n -= SBITMAP_ELT_BITS - shift;
- i++;
-
- /* Deal with full elts. */
- while (n >= SBITMAP_ELT_BITS)
-{
- if (bmap->elms[i])
- return false;
- i++;
- n -= SBITMAP_ELT_BITS;
-}
-
- /* The leftover bits. */
- if (n)
-{
- elm = bmap->elms[i];
- elm &= ((1 << n) - 1);
- return (elm == 0);
-}
-
- return true;
-}
-
-
/* Zero all elements in a bitmap. */
@@ -790,50 +733,3 @@ sbitmap_elt_popcount (SBITMAP_ELT_TYPE a
return ret;
}
#endif
-
-/* Count the number of bits in SBITMAP a, up to bit MAXBIT. */
-
-unsigned long
-sbitmap_popcount (const_sbitmap a, unsigned long maxbit)
-{
- unsigned long count = 0;
- unsigned ix;
- unsigned int lastword;
-
- if (maxbit == 0)
-return 0;
-
- if (maxbit >= a->n_bits)
-maxbit = a->n_bits;
-
- /* Count the bits in the full word. */
- lastword = MIN (a->size, SBITMAP_SET_SIZE (maxbit + 1) - 1);
- for (ix = 0; ix < lastword; ix++)
-{
- if (a->popcount)
- {
- count += a->popcount[ix];
-#ifdef BITMAP_DEBUGGING
- gcc_assert (a->popcount[ix] == do_popcount (a->elms[ix]));
-#endif
- }
- else
- count += do_popcount (a->elms[ix]);
-}
-
- /* Count the remaining bits. */
- if (lastword < a->size)
-{
- unsigned int bitindex;
- SBITMAP_ELT_TYPE theword = a->elms[lastword];
-
- bitindex = maxbit % SBITMAP_ELT_BITS;
- if (bitindex != 0)
- {
- theword &= (SBITMAP_ELT_TYPE)-1 >> (SBITMAP_ELT_BITS - bitindex);
- count += do_popcount (theword);
- }
-}
- return count;
-}
-
Index: gcc/sbi
[patch] Normalize bitmap iteration.
This patch renames sbitmap iterators to unify them with the bitmap iterators.
Remove the unused EXECUTE_IF_SET_IN_SBITMAP_REV, which has an unconventional
interface.
Rename the sbitmap_iter_* functions to match bitmap's bmp_iter_* functions.
Add an additional parameter to the initialization and next functions to
match the interface in bmp_iter_*. This extra parameter is mostly hidden
by the use of the EXECUTE_IF macros.
Rename the EXECUTE_IF_SET_IN_SBITMAP macro to EXECUTE_IF_SET_IN_BITMAP. Its
implementation is now identical to that in bitmap.h. To prevent redefinition
errors, both definitions are now guarded by #ifndef. An alternate strategy
is to simply include bitmap.h from sbitmap.h. As this would increase build
time, I have elected to use the #ifndef version. I do not have a strong
preference here.
The sbitmap_iterator type is still distinctly named because it is often
declared in contexts where the bitmap type is not obvious. There are less
than 40 uses of this type, so the burden to modify it when changing bitmap
types is not large.
Tested on x86-64, config-list.mk testing in progress.
Okay for trunk?
Index: gcc/ChangeLog
2012-10-31 Lawrence Crowl
* sbitmap.h (sbitmap_iter_init): Rename bmp_iter_set_init and add
unused parameter to match bitmap iterator. Update callers.
(sbitmap_iter_cond): Rename bmp_iter_set. Update callers.
(sbitmap_iter_next): Rename bmp_iter_next and add unused parameter to
match bitmap iterator. Update callers.
(EXECUTE_IF_SET_IN_SBITMAP_REV): Remove unused.
(EXECUTE_IF_SET_IN_SBITMAP): Rename EXECUTE_IF_SET_IN_BITMAP and
adjust to be identical to the definition in bitmap.h. Conditionalize
the definition based on not having been defined. Update callers.
* bitmap.h (EXECUTE_IF_SET_IN_BITMAP): Conditionalize the definition
based on not having been defined. (To match the above.)
Index: gcc/tree-into-ssa.c
===
--- gcc/tree-into-ssa.c (revision 193006)
+++ gcc/tree-into-ssa.c (working copy)
@@ -2672,12 +2672,12 @@ prepare_names_to_update (bool insert_phi
/* First process names in NEW_SSA_NAMES. Otherwise, uses of old
names may be considered to be live-in on blocks that contain
definitions for their replacements. */
- EXECUTE_IF_SET_IN_SBITMAP (new_ssa_names, 0, i, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (new_ssa_names, 0, i, sbi)
prepare_def_site_for (ssa_name (i), insert_phi_p);
/* If an old name is in NAMES_TO_RELEASE, we cannot remove it from
OLD_SSA_NAMES, but we have to ignore its definition site. */
- EXECUTE_IF_SET_IN_SBITMAP (old_ssa_names, 0, i, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (old_ssa_names, 0, i, sbi)
{
if (names_to_release == NULL || !bitmap_bit_p (names_to_release, i))
prepare_def_site_for (ssa_name (i), insert_phi_p);
@@ -2737,7 +2737,7 @@ dump_update_ssa (FILE *file)
fprintf (file, "N_i -> { O_1 ... O_j } means that N_i replaces "
"O_1, ..., O_j\n\n");
- EXECUTE_IF_SET_IN_SBITMAP (new_ssa_names, 0, i, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (new_ssa_names, 0, i, sbi)
dump_names_replaced_by (file, ssa_name (i));
}
@@ -3241,7 +3241,7 @@ update_ssa (unsigned update_flags)
for traversal. */
sbitmap tmp = sbitmap_alloc (SBITMAP_SIZE (old_ssa_names));
bitmap_copy (tmp, old_ssa_names);
- EXECUTE_IF_SET_IN_SBITMAP (tmp, 0, i, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (tmp, 0, i, sbi)
insert_updated_phi_nodes_for (ssa_name (i), dfs, blocks_to_update,
update_flags);
sbitmap_free (tmp);
@@ -3265,7 +3265,7 @@ update_ssa (unsigned update_flags)
/* Reset the current definition for name and symbol before renaming
the sub-graph. */
- EXECUTE_IF_SET_IN_SBITMAP (old_ssa_names, 0, i, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (old_ssa_names, 0, i, sbi)
get_ssa_name_ann (ssa_name (i))->info.current_def = NULL_TREE;
FOR_EACH_VEC_ELT (tree, symbols_to_rename, i, sym)
Index: gcc/sbitmap.c
===
--- gcc/sbitmap.c (revision 193006)
+++ gcc/sbitmap.c (working copy)
@@ -656,7 +656,7 @@ bitmap_first_set_bit (const_sbitmap bmap
unsigned int n = 0;
sbitmap_iterator sbi;
- EXECUTE_IF_SET_IN_SBITMAP (bmap, 0, n, sbi)
+ EXECUTE_IF_SET_IN_BITMAP (bmap, 0, n, sbi)
return n;
return -1;
}
Index: gcc/sbitmap.h
===
--- gcc/sbitmap.h (revision 193006)
+++ gcc/sbitmap.h (working copy)
@@ -45,7 +45,7 @@ along with GCC; see the file COPYING3.
* cardinality : sbitmap_popcount
* choose_one : bitmap_first_set_bit /
bitmap_last_set_bit
- * forall
Re: GCC 4.8.0 Status Report (2012-10-29), Stage 1 to end soon
On 10/30/12, Diego Novillo wrote: > On Mon, Oct 29, 2012 at 1:56 PM, Jakub Jelinek wrote: >> Status >> == >> >> I'd like to close the stage 1 phase of GCC 4.8 development >> on Monday, November 5th. If you have still patches for new features >> you'd >> like to see in GCC 4.8, please post them for review soon. Patches >> posted before the freeze, but reviewed shortly after the freeze, may >> still go in, further changes should be just bugfixes and documentation >> fixes. > > I will be committing the VEC overhaul soon. With any luck this week, > but PCH and gengtype are giving me a lot of grief. I have three remaining bitmap patches and the recently approved is_a/symtab/cgraph patch. However, Alexandre Oliva has a patch for bootstrap failure that is biting me. I can either incorporate it into my patches or wait for his patch and then submit. Comments? -- Lawrence Crowl
Re: [patch] Apply conditional down cast to cgraph.h et.al.
On 10/30/12, Diego Novillo wrote: > On Tue, Oct 30, 2012 at 4:53 PM, Lawrence Crowl wrote: >> On 10/30/12, Diego Novillo wrote: >>> >>> So, to use these three functions, the user must define this single >>> 'is_a_helper' routine? Nothing else? >> >> You need to distinguish which kind user. Someone just wanting >> to convert does not need to know about the is_a_helper stuff. >> Someone wanting to extend the set of type relationships needs to >> provide one or two template specializations. I've modified the >> in-header documentation to better reflect the distinction. > > Great. > >> I originally had >> >> if (cgraph_node *ce = dyn_cast (e)) >> if (!DECL_BUILT_IN (e->symbol.decl)) >> lto_cgraph_replace_node (ce, cgraph (prevailing)); >> >> but folks objected to increasing the nesting, and asked that I >> change to the pre-declare form. > > Ah, yeah. I remember that. OK, so we can now use both forms, right? Yes. I will commit the patch as soon as the merge and test is complete. -- Lawrence Crowl
Re: [patch] Apply conditional down cast to cgraph.h et.al.
On 10/30/12, Diego Novillo wrote:
> On 2012-10-29 15:01 , Lawrence Crowl wrote:
>> On 10/27/12, Marc Glisse wrote:
>>> On Fri, 26 Oct 2012, Lawrence Crowl wrote:
>>>> 2012-10-26 Lawrence Crowl >>
>>> missing '>'
>>
>> Fixed.
>>
>>>>* is-a.h: New.
>>>>(is_a (U*)): New. Test for is-a relationship.
>>>>(as_a (U*)): New. Treat as a derived type.
>>>>(dyn_cast (U*)): New. Conditionally cast based on is_a.
>>>
>>> I can't find this file in the patch...
>>
>> I forgot to svn add. Updated patch here.
>>
>>
>> This patch implements generic type query and conversion functions,
>> and applies them to the use of cgraph_node, varpool_node, and
>> symtab_node.
>>
>> The functions are:
>>
>> bool is_a (pointer)
>>Tests whether the pointer actually points to a more derived TYPE.
>>
>> TYPE *as_a (pointer)
>>Converts pointer to a TYPE*.
>>
>> TYPE *dyn_cast (pointer)
>>Converts pointer to TYPE* if and only if "is_a pointer".
>>Otherwise, returns NULL.
>>This function is essentially a checked down cast.
>>
>> These functions reduce compile time and increase type safety when treating
>> a
>> generic item as a more specific item. In essence, the code change is
>> from
>>
>>if (symtab_function_p (node))
>> {
>>struct cgraph_node *cnode = cgraph (node);
>>
>> }
>>
>> to
>>
>>if (cgraph_node *cnode = dyn_cast (node))
>> {
>>
>> }
>>
>> The necessary conditional test defines a variable that holds a known good
>> pointer to the specific item and avoids subsequent conversion calls and
>> the assertion checks that may come with them.
>>
>> When, the property test is embedded within a larger condition, the
>> variable
>> declaration gets pulled out of the condition. (This leaves some room for
>> using the variable inappropriately.)
>>
>>if (symtab_variable_p (node)
>>&& varpool (node)->finalized)
>> varpool_analyze_node (varpool (node));
>>
>> becomes
>>
>>varpool_node *vnode = dyn_cast (node);
>>if (vnode && vnode->finalized)
>> varpool_analyze_node (vnode);
>>
>> Note that we have converted two sets of assertions in the calls to
>> varpool
>> into safe and efficient use of a variable.
>>
>>
>> There are remaining calls to symtab_function_p and symtab_variable_p that
>> do not involve a pointer to a more specific type. These have been
>> converted
>> to calls to a functions is_a and is_a . The
>> original predicate functions have been removed.
>>
>> The cgraph.h header defined both a struct and a function with the name
>> varpool_node. This name overloading can cause some unintuitive error
>> messages
>> when, as is common in C++, one omits the struct keyword when using the
>> type.
>> I have renamed the function to varpool_node_for_decl.
>>
>> Tested on x86_64.
>>
>>
>> Okay for trunk?
>> Index: gcc/ChangeLog
>>
>> 2012-10-29 Lawrence Crowl
>>
>> * is-a.h: New.
>> (is_a (U*)): New. Test for is-a relationship.
>> (as_a (U*)): New. Treat as a derived type.
>> (dyn_cast (U*)): New. Conditionally cast based on is_a.
>> * cgraph.h (varpool_node): Rename to varpool_node_for_decl.
>> Adjust callers to match.
>> (is_a_helper ::test (symtab_node_def *)): New.
>> (is_a_helper ::test (symtab_node_def *)): New.
>> (symtab_node_def::try_function): New. Change most calls to
>> symtab_function_p with calls to dyn_cast (p).
>> (symtab_node_def::try_variable): New. Change most calls to
>> symtab_variable_p with calls to dyn_cast (p).
>> (symtab_function_p): Remove. Change callers to use
>> is_a (p) instead.
>> (symtab_variable_p): Remove. Change callers to use
>> is_a (p) instead.
>> * cgraph.c (cgraph_node_for_asm): Remove redundant call to
>> symtab_node_for_asm.
>> * cgraphunit.c (symbol_finalized_and_needed): New.
>> (symbol_finalized): New.
>> (cgraph_analyze_functions): Split complicated conditionals out into
>> above new functions.
>> * Makefile.in (CGRAPH_H): Add is-a.h as used by cgraph.h.
>
> Thanks. I really like this cleanup. I have a f
Re: [patch] Unify bitmap interface.
On 10/30/12, Diego Novillo wrote: > On Oct 30, 2012 Bin.Cheng wrote: > > Just one question: Should we change the name of functions > > "sbitmap_intersection_of_succs/sbitmap_intersection_of_preds/ > > sbitmap_union_of_succs/sbitmap_union_of_preds" too? It might > > be a little confusing that sbitmap_* is used among bitmap_*. > > Yes. Lawrence is proceeding with this unification in stages. > The next few patches should rename these. Actually, I didn't know about them because they are auxillary routines declared outside of the bitmap files. I've gone ahead and changed them as well. > The only two sets of functions that will remain separate for > now are the iterators and the bitmap creation routines, I think. > Lawrence? The iterator functions have been unified, but not the iterator type names. -- Lawrence Crowl
Re: [patch] Unify bitmap interface.
On 10/29/12, Diego Novillo wrote: > On Oct 29, 2012 Lawrence Crowl wrote: > > The sbitmap popcount function is only used in ebitmap, which is > > itself not used. If we do anything, removing them might be the > > thing to do. > > Yes, please. Separate patch, please. > > The bitmap.h iterators set and the sbitmap.h iterator set > > intersect in one element. I would rather treat the iterator > > conversion as a separate patch. > > OK, but this would be for 4.8, right? Yes, depending on when stage one closes. > > Richard said in an earlier mail, "I'd rather not mix this with > > any kind of further C++-ification (that is introduction of > > member functions or operator overloads)." So, exactly what > > are you all after? > > I'm after full unification. The patch seemed to stop short of > doing that. If you are going to do follow-up patches, that would > be fine. But stage 1 is closing, and I'd like to have this done > for 4.8. I want to minimize partial transitions. We have too > many of those already. The intent is for follow-up patches. > > Yes, I meant that. There are no functional changes here. > > Why is contrib/config-list.mk not sufficient? > > Just to make sure. Testing on ppc should be fast, for example. Okay. -- Lawrence Crowl
Re: [patch] Apply conditional down cast to cgraph.h et.al.
On 10/27/12, Marc Glisse wrote:
> On Fri, 26 Oct 2012, Lawrence Crowl wrote:
> > 2012-10-26 Lawrence Crowl
> missing '>'
Fixed.
> > * is-a.h: New.
> > (is_a (U*)): New. Test for is-a relationship.
> > (as_a (U*)): New. Treat as a derived type.
> > (dyn_cast (U*)): New. Conditionally cast based on is_a.
>
> I can't find this file in the patch...
I forgot to svn add. Updated patch here.
This patch implements generic type query and conversion functions,
and applies them to the use of cgraph_node, varpool_node, and symtab_node.
The functions are:
bool is_a (pointer)
Tests whether the pointer actually points to a more derived TYPE.
TYPE *as_a (pointer)
Converts pointer to a TYPE*.
TYPE *dyn_cast (pointer)
Converts pointer to TYPE* if and only if "is_a pointer".
Otherwise, returns NULL.
This function is essentially a checked down cast.
These functions reduce compile time and increase type safety when treating a
generic item as a more specific item. In essence, the code change is from
if (symtab_function_p (node))
{
struct cgraph_node *cnode = cgraph (node);
}
to
if (cgraph_node *cnode = dyn_cast (node))
{
}
The necessary conditional test defines a variable that holds a known good
pointer to the specific item and avoids subsequent conversion calls and
the assertion checks that may come with them.
When, the property test is embedded within a larger condition, the variable
declaration gets pulled out of the condition. (This leaves some room for
using the variable inappropriately.)
if (symtab_variable_p (node)
&& varpool (node)->finalized)
varpool_analyze_node (varpool (node));
becomes
varpool_node *vnode = dyn_cast (node);
if (vnode && vnode->finalized)
varpool_analyze_node (vnode);
Note that we have converted two sets of assertions in the calls to varpool
into safe and efficient use of a variable.
There are remaining calls to symtab_function_p and symtab_variable_p that
do not involve a pointer to a more specific type. These have been converted
to calls to a functions is_a and is_a . The
original predicate functions have been removed.
The cgraph.h header defined both a struct and a function with the name
varpool_node. This name overloading can cause some unintuitive error messages
when, as is common in C++, one omits the struct keyword when using the type.
I have renamed the function to varpool_node_for_decl.
Tested on x86_64.
Okay for trunk?
Index: gcc/ChangeLog
2012-10-29 Lawrence Crowl
* is-a.h: New.
(is_a (U*)): New. Test for is-a relationship.
(as_a (U*)): New. Treat as a derived type.
(dyn_cast (U*)): New. Conditionally cast based on is_a.
* cgraph.h (varpool_node): Rename to varpool_node_for_decl.
Adjust callers to match.
(is_a_helper ::test (symtab_node_def *)): New.
(is_a_helper ::test (symtab_node_def *)): New.
(symtab_node_def::try_function): New. Change most calls to
symtab_function_p with calls to dyn_cast (p).
(symtab_node_def::try_variable): New. Change most calls to
symtab_variable_p with calls to dyn_cast (p).
(symtab_function_p): Remove. Change callers to use
is_a (p) instead.
(symtab_variable_p): Remove. Change callers to use
is_a (p) instead.
* cgraph.c (cgraph_node_for_asm): Remove redundant call to
symtab_node_for_asm.
* cgraphunit.c (symbol_finalized_and_needed): New.
(symbol_finalized): New.
(cgraph_analyze_functions): Split complicated conditionals out into
above new functions.
* Makefile.in (CGRAPH_H): Add is-a.h as used by cgraph.h.
Index: gcc/is-a.h
===
--- gcc/is-a.h (revision 0)
+++ gcc/is-a.h (revision 0)
@@ -0,0 +1,118 @@
+/* Dynamic testing for abstract is-a relationships.
+ Copyright (C) 2012 Free Software Foundation, Inc.
+ Contributed by Lawrence Crowl.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+
+/*
+
+Suppose you have a symtab_node_def *ptr, AKA symtab_node ptr. You can
+test whether it points to a 'derived' cgraph_node as follows.
+
+ if (is_a (ptr))
+
+
+Y
Re: [patch] Unify bitmap interface.
On 10/29/12, Richard Biener wrote: > On Oct 29, 2012 Diego Novillo wrote: > > On Oct 25, 2012 Lawrence Crowl wrote: > > > The sbitmap non-bool returning bitwise operations have been > > > merged with the bool versions. Sometimes this merge involved > > > modifying the non-bool version to compute the bool value, > > > and sometimes modifying bool version to add additional work > > > from the non-bool version. The redundant routines have been > > > ifdef'd out. I will remove the ifdef'd out code later. > > > > No #if 0 code, please. Let's just remove them. Okay, but I point out that there is an awful lot of #if 0 code out there. I would rather have done such removal in a followup patch. > > > The allocation functions have not been renamed, because we > > > often do not have an argument on which to overload. > > > > Would it work if we made the first argument a reference to the > > bitmap being allocated? I suppose this shouldn't be a big deal. > > It's definitely good to at least in one place see what kind of > bitmap is actually being used ... ;) The parameters to the allocation functions are different. So even if they had the same name, they are not interchangable. > > > The cardinality functions have not been renamed, because they > > > have different parameters, and are thus not interchangable. > > > > Why not change the parameters then? > > Agree, as a followup maybe. The sbitmap popcount function is only used in ebitmap, which is itself not used. If we do anything, removing them might be the thing to do. > > > The iteration functions have not been renamed, because they > > > are functionally different. > > > > Functionally different? How? The bitmap.h iterators set and the sbitmap.h iterator set intersect in one element. I would rather treat the iterator conversion as a separate patch. > > It seems like the patch only goes skin deep. I would rather > > make a true unification. If the only thing that remains the > > different are the allocation functions, that's not a big deal. > > But the point was to make everything else the same. > > Indeed. It's not quite skin deep as it removes the distinction between the value returning and non-value returning functions. The intent here is to change the skin. Richard said in an earlier mail, "I'd rather not mix this with any kind of further C++-ification (that is introduction of member functions or operator overloads)." So, exactly what are you all after? > > I also notice that the patch does not rename any of the SET_BIT, > > RESET_BIT functions in sbitmap.c. > > They should go - a followup is fine though. That was the intent. > > > Tested on x86_64. Config testing in progress. > > > > You will also want to test on a couple other architecture on > > the farm. By config testing, do you mean contrib/config-list.mk? Yes, I meant that. There are no functional changes here. Why is contrib/config-list.mk not sufficient? -- Lawrence Crowl
Re: [patch] Unify bitmap interface.
On 10/25/12, Lawrence Crowl wrote: > This patch implements the unification of the *bitmap interfaces as > discussed. > Essentially, we rename ebitmap and sbitmap functions to use the same names > as the bitmap functions. This rename works because we can now overload > on the bitmap type. Some macros now become inline functions to enable > that overloading. > > The sbitmap non-bool returning bitwise operations have been merged with > the bool versions. Sometimes this merge involved modifying the non-bool > version to compute the bool value, and sometimes modifying bool version to > add additional work from the non-bool version. The redundant routines have > been ifdef'd out. I will remove the ifdef'd out code later. > > The allocation functions have not been renamed, because we often do not > have an argument on which to overload. The cardinality functions have not > been renamed, because they have different parameters, and are thus not > interchangable. The iteration functions have not been renamed, because > they are functionally different. > > Tested on x86_64. Config testing in progress. Config testing has only 3 failures, all unrelated. -- Lawrence Crowl
[patch] Apply conditional down cast to cgraph.h et.al.
This patch implements generic type query and conversion functions,
and applies them to the use of cgraph_node, varpool_node, and symtab_node.
The functions are:
bool is_a (pointer)
Tests whether the pointer actually points to a more derived TYPE.
TYPE *as_a (pointer)
Converts pointer to a TYPE*.
TYPE *dyn_cast (pointer)
Converts pointer to TYPE* if and only if "is_a pointer".
Otherwise, returns NULL.
This function is essentially a checked down cast.
These functions reduce compile time and increase type safety when treating a
generic item as a more specific item. In essence, the code change is from
if (symtab_function_p (node))
{
struct cgraph_node *cnode = cgraph (node);
}
to
if (cgraph_node *cnode = dyn_cast (node))
{
}
The necessary conditional test defines a variable that holds a known good
pointer to the specific item and avoids subsequent conversion calls and
the assertion checks that may come with them.
When, the property test is embedded within a larger condition, the variable
declaration gets pulled out of the condition. (This leaves some room for
using the variable inappropriately.)
if (symtab_variable_p (node)
&& varpool (node)->finalized)
varpool_analyze_node (varpool (node));
becomes
varpool_node *vnode = dyn_cast (node);
if (vnode && vnode->finalized)
varpool_analyze_node (vnode);
Note that we have converted two sets of assertions in the calls to varpool
into safe and efficient use of a variable.
There are remaining calls to symtab_function_p and symtab_variable_p that
do not involve a pointer to a more specific type. These have been converted
to calls to a functions is_a and is_a . The
original predicate functions have been removed.
The cgraph.h header defined both a struct and a function with the name
varpool_node. This name overloading can cause some unintuitive error messages
when, as is common in C++, one omits the struct keyword when using the type.
I have renamed the function to varpool_node_for_decl.
Tested on x86_64.
Okay for trunk?
Index: gcc/ChangeLog
2012-10-26 Lawrence Crowl (U*)): New. Test for is-a relationship.
(as_a (U*)): New. Treat as a derived type.
(dyn_cast (U*)): New. Conditionally cast based on is_a.
* cgraph.h (varpool_node): Rename to varpool_node_for_decl.
Adjust callers to match.
(is_a_helper ::test (symtab_node_def *)): New.
(is_a_helper ::test (symtab_node_def *)): New.
(symtab_node_def::try_function): New. Change most calls to
symtab_function_p with calls to dyn_cast (p).
(symtab_node_def::try_variable): New. Change most calls to
symtab_variable_p with calls to dyn_cast (p).
(symtab_function_p): Remove. Change callers to use
is_a (p) instead.
(symtab_variable_p): Remove. Change callers to use
is_a (p) instead.
* cgraph.c (cgraph_node_for_asm): Remove redundant call to
symtab_node_for_asm.
* cgraphunit.c (symbol_finalized_and_needed): New.
(symbol_finalized): New.
(cgraph_analyze_functions): Split complicated conditionals out into
above new functions.
* Makefile.in (CGRAPH_H): Add is-a.h as used by cgraph.h.
Index: gcc/lto-symtab.c
===
--- gcc/lto-symtab.c(revision 192862)
+++ gcc/lto-symtab.c(working copy)
@@ -532,11 +532,11 @@ lto_symtab_merge_cgraph_nodes_1 (symtab_
if (!symtab_real_symbol_p (e))
continue;
- if (symtab_function_p (e)
- && !DECL_BUILT_IN (e->symbol.decl))
- lto_cgraph_replace_node (cgraph (e), cgraph (prevailing));
- if (symtab_variable_p (e))
- lto_varpool_replace_node (varpool (e), varpool (prevailing));
+ cgraph_node *ce = dyn_cast (e);
+ if (ce && !DECL_BUILT_IN (e->symbol.decl))
+ lto_cgraph_replace_node (ce, cgraph (prevailing));
+ if (varpool_node *ve = dyn_cast (e))
+ lto_varpool_replace_node (ve, varpool (prevailing));
}
return;
Index: gcc/cgraphbuild.c
===
--- gcc/cgraphbuild.c (revision 192862)
+++ gcc/cgraphbuild.c (working copy)
@@ -84,7 +84,7 @@ record_reference (tree *tp, int *walk_su
if (TREE_CODE (decl) == VAR_DECL)
{
- struct varpool_node *vnode = varpool_node (decl);
+ struct varpool_node *vnode = varpool_node_for_decl (decl);
ipa_record_reference ((symtab_node)ctx->varpool_node,
(symtab_node)vnode,
IPA_REF_ADDR, NULL);
@@ -123,7 +123,7 @@ record_type_list (struct cgraph_node *no
type = TREE_OPERAND (type, 0);
if (TREE_CODE (type) == VAR_DECL)
{
- struct varpool_node *vnode = varpool_node (type);
+
Re: Change hash_table for separate comparator, documentation, cleanups...
On 10/26/12, Richard Biener wrote: > On Oct 25, 2012 Lawrence Crowl wrote: > > Change hash_table to support a comparator type different from the > > value type stored in the hash table. The 'find' functions now > > may take a different type from the value type. This requires > > introducing a second typedef into the Descriptor conceptual type. > > Change the Descriptor concept to use typedefs value_type and > > compare_type instead of T. Change all users to match. > > > > Add usage documentation to hash-table.h. > > > > Tested on x86-64. > > > > Okay for trunk? > > Can you elaborate on why this is needed? There are several uses of htab_t that have a different type for the comparator than from the values in the table. I also got a question asking how to make that happen. So, to enable handling the existing cases and the question, I added the capability. -- Lawrence Crowl
Change hash_table for separate comparator, documentation, cleanups...
Change hash_table to support a comparator type different from the
value type stored in the hash table. The 'find' functions now may
take a different type from the value type. This requires introducing
a second typedef into the Descriptor conceptual type. Change the
Descriptor concept to use typedefs value_type and compare_type instead
of T. Change all users to match.
Add usage documentation to hash-table.h.
Tested on x86-64.
Okay for trunk?
Index: gcc/ChangeLog
2012-10-25 Lawrence Crowl
* hash-table.h: Add usage documentation.
(template struct typed_free_remove): Clarify documentation.
Rename template parameter.
(struct typed_noop_remove): Likewise.
(descriptor concept): Change typedef T to value_type.
Add typedef compare_type. Use more precise template parameter name,
Descriptor instead of Descr. Update users to match.
(struct hash_table): Change 'find' parameters to use compare_type
instead of the value type.
Index: gcc/tree-ssa-tail-merge.c
===
--- gcc/tree-ssa-tail-merge.c (revision 192820)
+++ gcc/tree-ssa-tail-merge.c (working copy)
@@ -226,10 +226,11 @@ struct same_succ_def
hashval_t hashval;
/* hash_table support. */
- typedef same_succ_def T;
- static inline hashval_t hash (const same_succ_def *);
- static int equal (const same_succ_def *, const same_succ_def *);
- static void remove (same_succ_def *);
+ typedef same_succ_def value_type;
+ typedef same_succ_def compare_type;
+ static inline hashval_t hash (const value_type *);
+ static int equal (const value_type *, const compare_type *);
+ static void remove (value_type *);
};
typedef struct same_succ_def *same_succ;
typedef const struct same_succ_def *const_same_succ;
@@ -237,7 +238,7 @@ typedef const struct same_succ_def *cons
/* hash routine for hash_table support, returns hashval of E. */
inline hashval_t
-same_succ_def::hash (const same_succ_def *e)
+same_succ_def::hash (const value_type *e)
{
return e->hashval;
}
@@ -528,7 +529,7 @@ inverse_flags (const_same_succ e1, const
/* Compares SAME_SUCCs E1 and E2. */
int
-same_succ_def::equal (const_same_succ e1, const_same_succ e2)
+same_succ_def::equal (const value_type *e1, const compare_type *e2)
{
unsigned int i, first1, first2;
gimple_stmt_iterator gsi1, gsi2;
Index: gcc/tree-ssa-threadupdate.c
===
--- gcc/tree-ssa-threadupdate.c (revision 192820)
+++ gcc/tree-ssa-threadupdate.c (working copy)
@@ -127,20 +127,21 @@ struct redirection_data : typed_free_rem
struct el *incoming_edges;
/* hash_table support. */
- typedef redirection_data T;
- static inline hashval_t hash (const redirection_data *);
- static inline int equal (const redirection_data *, const redirection_data
*);
+ typedef redirection_data value_type;
+ typedef redirection_data compare_type;
+ static inline hashval_t hash (const value_type *);
+ static inline int equal (const value_type *, const compare_type *);
};
inline hashval_t
-redirection_data::hash (const redirection_data *p)
+redirection_data::hash (const value_type *p)
{
edge e = p->outgoing_edge;
return e->dest->index;
}
inline int
-redirection_data::equal (const redirection_data *p1, const
redirection_data *p2)
+redirection_data::equal (const value_type *p1, const compare_type *p2)
{
edge e1 = p1->outgoing_edge;
edge e2 = p2->outgoing_edge;
Index: gcc/java/jcf-io.c
===
--- gcc/java/jcf-io.c (revision 192820)
+++ gcc/java/jcf-io.c (working copy)
@@ -276,19 +276,21 @@ find_classfile (char *filename, JCF *jcf
struct charstar_hash : typed_noop_remove
{
- typedef const char T;
- static inline hashval_t hash (const T *candidate);
- static inline bool equal (const T *existing, const T *candidate);
+ typedef const char value_type;
+ typedef const char compare_type;
+ static inline hashval_t hash (const value_type *candidate);
+ static inline bool equal (const value_type *existing,
+ const compare_type *candidate);
};
inline hashval_t
-charstar_hash::hash (const T *candidate)
+charstar_hash::hash (const value_type *candidate)
{
return htab_hash_string (candidate);
}
inline bool
-charstar_hash::equal (const T *existing, const T *candidate)
+charstar_hash::equal (const value_type *existing, const compare_type
*candidate)
{
return strcmp (existing, candidate) == 0;
}
Index: gcc/valtrack.h
===
--- gcc/valtrack.h (revision 192820)
+++ gcc/valtrack.h (working copy)
@@ -46,32 +46,33 @@ struct dead_debug_global_entry
struct dead_debug_hash_descr
{
/* The hash table contains pointers to entries of this type. */
- typedef struct dead_debug_global_entry T;
+
Re: patch to fix constant math - 4th patch - the wide-int class.
On 10/23/12, Kenneth Zadeck wrote:
> On 10/23/2012 02:38 PM, Lawrence Crowl wrote:
>> On 10/23/12, Kenneth Zadeck wrote:
>>> On 10/23/2012 10:12 AM, Richard Biener wrote:
>>>> + inline bool minus_one_p () const;
>>>> + inline bool zero_p () const;
>>>> + inline bool one_p () const;
>>>> + inline bool neg_p () const;
>>>>
>>>> what's wrong with w == -1, w == 0, w == 1, etc.?
>>> I would love to do this and you seem to be somewhat knowledgeable
>>> of c++. But i cannot for the life of me figure out how to do it.
>> Starting from the simple case, you write an operator ==.
>>
>> as global operator: bool operator == (wide_int w, int i);
>> as member operator: bool wide_int::operator == (int i);
>>
>> In the simple case,
>>
>> bool operator == (wide_int w, int i)
>> {
>>switch (i)
>> {
>>case -1: return w.minus_one_p ();
>>case 0: return w.zero_p ();
>>case 1: return w.one_p ();
>>default: unexpected
>> }
>> }
>
> no, this seems wrong.you do not want to write code that can only
> fail at runtime unless there is a damn good reason to do that.
Well, that's because it's the oversimplified case. :-)
>>> say i have a TImode number, which must be represented in 4 ints
>>> on a 32 bit host (the same issue happens on 64 bit hosts, but
>>> the examples are simpler on 32 bit hosts) and i compare it to -1.
>>> The value that i am going to see as the argument of the function
>>> is going have the value 0x. but the value that i have
>>> internally is 128 bits. do i take this and 0 or sign extend it?
>>
>> What would you have done with w.minus_one_p ()?
>
> the code "knows" that -1 is a negative number and it knows the
> precision of w. That is enough information. So it logically
> builds a -1 that has enough bits to do the conversion.
And the code could also know that '-n' is a negative number and do
the identical conversion. It would certainly be more difficult to
write and get all the edge cases.
>>> in particular if someone wants to compare a number to 0xdeadbeef i
>>> have no idea what to do. I tried defining two different functions,
>>> one that took a signed and one that took and unsigned number but
>>> then i wanted a cast in front of all the positive numbers.
>> This is where it does get tricky. For signed arguments, you should sign
>> extend. For unsigned arguments, you should not. At present, we need
>> multiple overloads to avoid type ambiguities.
>>
>> bool operator == (wide_int w, long long int i);
>> bool operator == (wide_int w, unsigned long long int i);
>> inline bool operator == (wide_int w, long int i)
>>{ return w == (long long int) i; }
>> inline bool operator (wide_int w, unsigned long int i)
>>{ return w == (unsigned long long int) i; }
>> inline bool operator == (wide_int w, int i)
>>{ return w == (long long int) i; }
>> inline bool operator (wide_int w, unsigned int i)
>>{ return w == (unsigned long long int) i; }
>>
>> (There is a proposal before the C++ committee to fix this problem.)
>>
>> Even so, there is room for potential bugs when wide_int does not
>> carry around whether or not it is signed. The problem is that
>> regardless of what the programmer thinks of the sign of the wide int,
>> the comparison will use the sign of the int.
>
> when they do we can revisit this. but i looked at this and i said the
> potential bugs were not worth the effort.
I won't disagree. I was answering what I thought were questions on
what was possible.
>>> If there is a way to do this, then i will do it, but it is going
>>> to have to work properly for things larger than a HOST_WIDE_INT.
>> The long-term solution, IMHO, is to either carry the sign information
>> around in either the type or the class data. (I prefer type, but
>> with a mechanism to carry it as data when needed.) Such comparisons
>> would then require consistency in signedness between the wide int
>> and the plain int.
>
> carrying the sign information is a non starter.The rtl level does
> not have it and the middle end violates it more often than not.My
> view was to design this having looked at all of the usage. I have
> basically converted the whole compiler before i released the abi. I am
> still getting out the errors and breaking it up in reviewable sized
> patches, but i knew very very well who my clients were before i wrote
> the abi.
Okay.
Re: patch to fix constant math - 4th patch - the wide-int class.
On 10/23/12, Kenneth Zadeck wrote:
> On 10/23/2012 10:12 AM, Richard Biener wrote:
> > + inline bool minus_one_p () const;
> > + inline bool zero_p () const;
> > + inline bool one_p () const;
> > + inline bool neg_p () const;
> >
> > what's wrong with w == -1, w == 0, w == 1, etc.?
>
> I would love to do this and you seem to be somewhat knowledgeable
> of c++. But i cannot for the life of me figure out how to do it.
Starting from the simple case, you write an operator ==.
as global operator: bool operator == (wide_int w, int i);
as member operator: bool wide_int::operator == (int i);
In the simple case,
bool operator == (wide_int w, int i)
{
switch (i)
{
case -1: return w.minus_one_p ();
case 0: return w.zero_p ();
case 1: return w.one_p ();
default: unexpected
}
}
> say i have a TImode number, which must be represented in 4 ints
> on a 32 bit host (the same issue happens on 64 bit hosts, but
> the examples are simpler on 32 bit hosts) and i compare it to -1.
> The value that i am going to see as the argument of the function
> is going have the value 0x. but the value that i have
> internally is 128 bits. do i take this and 0 or sign extend it?
What would you have done with w.minus_one_p ()?
> in particular if someone wants to compare a number to 0xdeadbeef i
> have no idea what to do. I tried defining two different functions,
> one that took a signed and one that took and unsigned number but
> then i wanted a cast in front of all the positive numbers.
This is where it does get tricky. For signed arguments, you should sign
extend. For unsigned arguments, you should not. At present, we need
multiple overloads to avoid type ambiguities.
bool operator == (wide_int w, long long int i);
bool operator == (wide_int w, unsigned long long int i);
inline bool operator == (wide_int w, long int i)
{ return w == (long long int) i; }
inline bool operator (wide_int w, unsigned long int i)
{ return w == (unsigned long long int) i; }
inline bool operator == (wide_int w, int i)
{ return w == (long long int) i; }
inline bool operator (wide_int w, unsigned int i)
{ return w == (unsigned long long int) i; }
(There is a proposal before the C++ committee to fix this problem.)
Even so, there is room for potential bugs when wide_int does not
carry around whether or not it is signed. The problem is that
regardless of what the programmer thinks of the sign of the wide int,
the comparison will use the sign of the int.
> If there is a way to do this, then i will do it, but it is going
> to have to work properly for things larger than a HOST_WIDE_INT.
The long-term solution, IMHO, is to either carry the sign information
around in either the type or the class data. (I prefer type, but
with a mechanism to carry it as data when needed.) Such comparisons
would then require consistency in signedness between the wide int
and the plain int.
> I know that double-int does some of this and it does not carry
> around a notion of signedness either. is this just code that has
> not been fully tested or is there a trick in c++ that i am missing?
The double int class only provides == and !=, and only with other
double ints. Otherwise, it has the same value query functions that
you do above. In the case of double int, the goal was to simplify
use of the existing semantics. If you are changing the semantics,
consider incorporating sign explicitly.
--
Lawrence Crowl
Re: patch to fix constant math - 4th patch - the wide-int class.
On 10/23/12, Richard Biener wrote:
> I wonder if for the various ways to specify precision/len there
> is a nice C++ way of moving this detail out of wide-int. I can
> think only of one:
>
> struct WIntSpec {
> WIntSpec (unsigned int len, unsigned int precision);
> WIntSpec (const_tree);
> WIntSpec (enum machine_mode);
> unsigned int len;
> unsigned int precision;
> };
>
> and then (sorry to pick one of the less useful functions):
>
> inline static wide_int zero (WIntSpec)
>
> which you should be able to call like
>
> wide_int::zero (SImode)
> wide_int::zero (integer_type_node)
>
> and (ugly)
>
> wide_int::zero (WIntSpec (32, 32))
>
> with C++0x wide_int::zero ({32, 32}) should be possible? Or we
> keep the precision overload. At least providing the WIntSpec
> abstraction allows custom ways of specifying required bits to
> not pollute wide-int itself too much. Lawrence?
Yes, in C++11, wide_int::zero ({32, 32}) is possible using an
implicit conversion to WIntSpec from an initializer_list. However,
at present we are limited to C++03 to enable older compilers as
boot compilers.
--
Lawrence Crowl
Re: wide int patch #6: Replacement of hwi extraction from int-csts.
On 10/19/12, Richard Biener wrote: > The existing tree_low_cst function performs checking, so > tree_to_hwi should as well. > > I don't think mismatch of signedness of the variable assigned to > with the sign we use for hwi extraction is any good. C++ isn't > type-safe here for the return value but if we'd use a reference > as return slot we could make it so ... (in exchange for quite > some ugliness IMNSHO): > > void tree_to_shwi (const_tree tree, HOST_WIDE_INT &hwi); > > vs. > > void tree_to_uhwi (const_tree tree, unsigned HOST_WIDE_INT &hwi); > > maybe more natural would be > > void hwi_from_tree (HOST_WIDE_INT &hwi, const_tree tree); > void hwi_from_tree (unsigned HOST_WIDE_INT &hwi, const_tree tree); > > let the C++ bikeshedding begin! (the point is to do appropriate > checking for a conversion of (INTEGER_CST) tree to HOST_WIDE_INT > vs. unsigned HOST_WIDE_INT) We could add conversion operators to achieve the effect. However, we probably don't want to do so until we can make them explicit. Unfortunately, explicit conversion operators are not available until C++11. > No, I don't want you to do the above transform with this patch ;) -- Lawrence Crowl
Re: [PATCH][RFC] Re-organize how we stream trees in LTO
On 10/16/12, Diego Novillo wrote: > On 2012-10-16 10:43 , Richard Biener wrote: > > Diego - is PTH still live? Thus, do I need to bother about > > inventing things in a way that can be hook-ized? > > We will eventually revive PPH. But not in the short term. I think > it will come back when/if we start implementing C++ modules. > Jason, Lawrence, is that something that you see coming for the > next standard? There are some people working on it, though not very publically. Many folks would like to see modules in the next full standard, probably circa 2017. It is likely that the design point for standard modules will differ from PPH, and so I don't think that the current PPH implementation should serve as a constraint on other work. > I suspect that the front end will need to distance itself from > 'tree' and have its own streamable IL. So, the hooks may not be > something we need to keep long term. > > Emitting the trees in SCC groups should not affect the C++ > streamer too much. It already is doing its own strategy of > emitting tree headers so it can do declaration and type merging. > As long as the trees can be fully materialized from the SCC groups, > it should be fine. -- Lawrence Crowl
Re: Add usage documentation for hash_table
gt; @@ -342,8 +445,7 @@ hash_table ::collisions()
>
> /* Create a hash table with at least the given number of INITIAL_SLOTS.
> */
>
> -template - template class Allocator>
> +template class Allocator>
> void
> hash_table ::create (size_t size)
> {
> @@ -364,8 +466,7 @@ hash_table ::create (size_t size)
> /* Dispose of a hash table. Free all memory and return this hash table to
> the non-created state. Naturally the hash table must already exist.
> */
>
> -template - template class Allocator>
> +template class Allocator>
> void
> hash_table ::dispose ()
> {
> @@ -389,11 +490,9 @@ hash_table ::dispose ()
> This function also assumes there are no deleted entries in the table.
> HASH is the hash value for the element to be inserted. */
>
> -template - template class Allocator>
> +template class Allocator>
> typename Descr::T **
> -hash_table
> -::find_empty_slot_for_expand (hashval_t hash)
> +hash_table ::find_empty_slot_for_expand (hashval_t hash)
> {
>hashval_t index = hash_table_mod1 (hash, htab->size_prime_index);
>size_t size = htab->size;
> @@ -428,8 +527,7 @@ hash_table
> table entries is changed. If memory allocation fails, this function
> will abort. */
>
> -template - template class Allocator>
> +template class Allocator>
> void
> hash_table ::expand ()
> {
> @@ -491,11 +589,10 @@ hash_table ::expand ()
> COMPARABLE element starting with the given HASH value. It cannot
> be used to insert or delete an element. */
>
> -template - template class Allocator>
> +template class Allocator>
> typename Descr::T *
> -hash_table
> -::find_with_hash (const T *comparable, hashval_t hash)
> +hash_table ::find_with_hash (const T *comparable,
> +hashval_t hash)
> {
>hashval_t index, hash2;
>size_t size;
> @@ -534,12 +631,11 @@ hash_table
> write the value you want into the returned slot. When inserting an
> entry, NULL may be returned if memory allocation fails. */
>
> -template - template class Allocator>
> +template class Allocator>
> typename Descr::T **
> -hash_table
> -::find_slot_with_hash (const T *comparable, hashval_t hash,
> -enum insert_option insert)
> +hash_table ::find_slot_with_hash (const T *comparable,
> + hashval_t hash,
> + enum insert_option insert)
> {
>T **first_deleted_slot;
>hashval_t index, hash2;
> @@ -565,7 +661,7 @@ hash_table
> first_deleted_slot = &htab->entries[index];
>else if (Descr::equal (entry, comparable))
> return &htab->entries[index];
> -
> +
>hash2 = hash_table_mod2 (hash, htab->size_prime_index);
>for (;;)
> {
> @@ -573,7 +669,7 @@ hash_table
>index += hash2;
>if (index >= size)
> index -= size;
> -
> +
>entry = htab->entries[index];
>if (entry == HTAB_EMPTY_ENTRY)
> goto empty_entry;
> @@ -639,15 +735,15 @@ hash_table ::empty ()
> useful when you've already done the lookup and don't want to do it
> again. */
>
> -template - template class Allocator>
> +template class Allocator>
> void
> -hash_table
> -::clear_slot (T **slot)
> +hash_table ::clear_slot (T **slot)
> {
> - if (slot < htab->entries || slot >= htab->entries + htab->size
> - || *slot == HTAB_EMPTY_ENTRY || *slot == HTAB_DELETED_ENTRY)
> -abort ();
> + if (slot < htab->entries
> + || slot >= htab->entries + htab->size
> + || *slot == HTAB_EMPTY_ENTRY
> + || *slot == HTAB_DELETED_ENTRY)
> +gcc_unreachable ();
>
>Descr::remove (*slot);
>
> @@ -660,11 +756,10 @@ hash_table
> from hash table starting with the given HASH. If there is no
> matching element in the hash table, this function does nothing. */
>
> -template - template class Allocator>
> +template class Allocator>
> void
> -hash_table
> -::remove_elt_with_hash (const T *comparable, hashval_t hash)
> +hash_table ::remove_elt_with_hash (const T *comparable,
> + hashval_t hash)
> {
>T **slot;
>
> @@ -683,13 +778,11 @@ hash_table
> each live entry. If CALLBACK returns false, the iteration stops.
> ARGUMENT is passed as CALLBACK's second argument. */
>
> -template - template class Allocator>
> +template class Allocator>
> template int (*Callback) (typename Descr::T **slot, Argument argument)>
> void
> -hash_table
> -::traverse_noresize (Argument argument)
> +hash_table ::traverse_noresize (Argument argument)
> {
>T **slot;
>T **limit;
> @@ -712,13 +805,11 @@ hash_table
> /* Like traverse_noresize, but does resize the table when it is too empty
> to improve effectivity of subsequent calls. */
>
> -template - template class Allocator>
> +template class Allocator>
> template int (*Callback) (typename Descr::T **slot, Argument argument)>
> void
> -hash_table
> -::traverse (Argument argument)
> +hash_table ::traverse (Argument argument)
> {
>size_t size = htab->size;
>if (elements () * 8 < size && size > 32)
>
--
Lawrence Crowl
Re: Use conditional casting with symtab_node
On 10/12/12, Richard Biener wrote:
> On Oct 11, 2012 Xinliang David Li wrote:
>> On Oct 11, 2012 Lawrence Crowl wrote:
>>> On 10/10/12, Xinliang David Li wrote:
>>>> In a different thread, I proposed the following alternative to
>>>> 'try_xxx':
>>>>
>>>> template T* symbol::cast_to(symbol* p) {
>>>>if (p->is())
>>>> return static_cast(p);
>>>>return 0;
>>>> }
>>>>
>>>> cast:
>>>>
>>>> template T& symbol:as(symbol* p) {
>>>>assert(p->is())
>>>>return static_cast(*p);
>>>>
>>>> }
>>>
>>> My concern here was never the technical feasibility, nor the
>>> desirability of having the facility for generic code, but the amount
>>> of the amount of typing in non-generic contexts. That is
>>>
>>> if (cgraph_node *q = p->try_function ())
>>>
>>> versus
>>>
>>> if (cgraph_node *q = p->cast_to ())
>>>
>>> I thought the latter would generate objections. Does anyone object
>>> to the extra typing?
>>>
>>> If not, I can make that change.
>>
>> Think about a more complex class hierarchy and interface consistency.
>> I believe you don't want this:
>>
>> tree::try_decl () { .. }
>> tree::try_ssa_name () { ..}
>> tree::try_type() {...}
>> tree::try_int_const () {..}
>> tree::try_exp () { ...}
>> ..
>>
>> Rather one (or two with the const_cast version).
>>
>> template T *tree::cast_to ()
>> {
>>if (kind_ == kind_traits::value)
>> return static_cast (this);
>>
>> return 0;
>> }
>
> I also think that instead of
>
>>> if (cgraph_node *q = p->cast_to ())
>
> we want
>
> if ((q = cast_to (p))
>
> I see absolutely no good reason to make cast_to a member, given
> that the language has static_cast, const_cast and stuff. cast_to
> would simply be our equivalent to dynamic_cast within our OO model.
Sorry, that was a think-o. Agreed there is no point in it being a
member.
> Then I'd call it *_cast instead of cast_*, so, why not gcc_cast < >?
> Or dyn_cast <> (). That way
>
> if ((q = dyn_cast (p))
>
> would be how to use it.
Which function? The point with my original proposal is that the
kind of function was derivable from context, and thus can be shorter.
> Small enough, compared to
>
> if ((q = p->try_function ()))
>
> and a lot more familiar to folks knowing C++ (and probably doesn't
> make a difference to others).
>
> Thus, please re-use or follow existing concepts.
Both are existing concepts. What I proposed is a common technique
for avoiding the cost of dynamic_cast when down casting in a class
hierarchy. Both concepts will work. I proposed what I thought
would be most convenient to programmers. However, I will change
to the other form unless someone objects.
> As for the example with tree we'd then have
>
> if ((q = dyn_cast (p)))
>
> if we can overload on the template parameter kind (can we?
> typename vs. enum?)
There are two template parameters, one for the argument type and
one for the return type. We can overload on the argument type but
not on the return type. We can, however, (indirectly) partially
specialize on the return type. We need to do that anyway to
represent the fact that not all type conversions are legal.
However, I recommend against specializing on the enum, as it would
become somewhat obscure when the hierarchy is discriminated on more
than one enum.
> or use tree_cast <> (no I don't want dyn_cast
> all around the code).
Once we have proper class hierarchies, derived to base conversions
are implicit. In the meantime, we need something else.
--
Lawrence Crowl
Re: Use conditional casting with symtab_node
On 10/10/12, Xinliang David Li wrote:
> In a different thread, I proposed the following alternative to 'try_xxx':
>
> template T* symbol::cast_to(symbol* p) {
>if (p->is())
> return static_cast(p);
>return 0;
> }
>
> cast:
>
> template T& symbol:as(symbol* p) {
>assert(p->is())
>return static_cast(*p);
>
> }
My concern here was never the technical feasibility, nor the
desirability of having the facility for generic code, but the amount
of the amount of typing in non-generic contexts. That is
if (cgraph_node *q = p->try_function ())
versus
if (cgraph_node *q = p->cast_to ())
I thought the latter would generate objections. Does anyone object
to the extra typing?
If not, I can make that change.
--
Lawrence Crowl
Re: Use conditional casting with symtab_node
On 10/5/12, Jan Hubicka wrote:
>> On Thu, Oct 4, 2012 at 8:16 PM, Diego Novillo wrote:
>> > On Thu, Oct 4, 2012 at 2:14 PM, Lawrence Crowl wrote:
>> >
>> >> So, Jan Hubicka requested and approved the current spelling.
>> >> What now?
>> >
>> > I don't think we should hold this up. The names Jan requested seem
>> > reasonable enough. We seem to be running in circles here.
>>
>> I suppose I have your promise that we'll release with consistent names.
>> Please allocate some work hours on your side for the renaming of
>> cgraph_node and varpool_node for the case Honza doesn't get to it in
>> time.
>
> Not that I would not agree with Richard with most of his points. To be
> however
> fair here, I only asked to continue naming I already introduced in:
>
> /* Return true when NODE is function. */
> static inline bool
> symtab_function_p (symtab_node node)
> {
> return node->symbol.type == SYMTAB_FUNCTION;
> }
>
> /* Return true when NODE is variable. */
> static inline bool
> symtab_variable_p (symtab_node node)
> {
> return node->symbol.type == SYMTAB_VARIABLE;
> }
>
> Even if variable are represented by varpool and functions by cgraph, I do
> not
> see these names more confusing compared to
> symtab_cgraph_p/symtab_varpool_p.
> The most common use is when you walk the symbol table and you want to
> handle
> functions and variables specially.
>
> The new interface with try_ scheme gives a bit more inconsistent feeling,
> but it is just an surface, nothing really changes.
>
> I am not happy with current naming scheme and also with the fact that for
> gengtype reasons we also have symtab_node typedef, but for varpool and
> cgraph
> we use struct (this is because symtab_node has to be union without GTY
> supporting inheritance).
>
> Introducing symtab I did not have much other options and I expected that
> at the end of this stage1 I will clean it up. This is, well, more or less
> now
> when assuming that there are no major patches to this area just to appear
> for this stage1.
>
> I guess we all agreed we want to drop cgraph/varpool nodes in favor of
> functions/ variables. How realistic is for gengtype to support inheritance
> this release cycle? I would really like to see these three turned into
> classes
> with the inheritance this release cycle. Renaming them during the process
> should be easy and just a nice bonus.
I would like some clarity. Can I commit this patch?
--
Lawrence Crowl
Re: Convert more non-GTY htab_t to hash_table.
On 10/5/12, Diego Novillo wrote: > On Oct 3, 2012 Lawrence Crowl wrote: > > Change more non-GTY hash tables to use the new type-safe > > template hash table. Constify member function parameters that > > can be const. Correct a couple of expressions in formerly > > uninstantiated templates. > > > > The new code is 0.362% faster in bootstrap, with a 99.5% > > confidence of being faster. > > > > Tested on x86-64. > > > > Okay for trunk? > > Given that the changes to the front ends are mechanical and a > side-effect of the main hash table changes, I think they should > be OK without further review (provided tests pass of course). > > The changes look fine to me. To be extra safe, let's wait a couple > more days to give the FE maintainers a chance to look at the patch. Committed. -- Lawrence Crowl
Re: Convert more non-GTY htab_t to hash_table.
On 10/5/12, Jakub Jelinek wrote: > On Fri, Oct 05, 2012 at 01:59:18PM -0700, Lawrence Crowl wrote: > > With the constructor, you don't have to remember and you don't > > have to type more. If you have a variable, you know that it is > > properly initialized. > > But we really don't want hundreds or thousands of dynamic > constructors for global variables. Nothing I have written requires a dynamic initializer for global variables. It may be that a dynamic initializer is the easiest implementation. If the cost is significant, we can fix it by applying the optimization, which would benefit our customers in addition to us. > For many people the time to compile (almost) empty file is very > important, we are already bad about that right now, initializing > too much stuff dynamically is going to make it worse. So far, we are looking at dynamic initializations that would take about 10 cycles. Even on a slow processor, a thousand initializations would take a microsecond. Our time reports don't even report anything less than 5 milliseconds. Is there any reason to believe that this anticipated static initialization overhead is not pretty low relative to other overhead? I'm thinking here of the fact that to even start, the driver launches cc1[plus] which has to parse all the options created by the driver. -- Lawrence Crowl
Re: Use conditional casting with symtab_node
On 10/5/12, Diego Novillo wrote: > On Oct 5, 2012 Richard Guenther wrote: > > Sorry, that wasn't intended. I question these numbers because > > unless you bootstrap say 100 times the noise in bootstrap > > speed is way too high to make such claims. Of course critical > > information is missing: > > I agree with Nathan. Your tone is sometimes borderline insulting. > It creates unnecessary friction and does not serve anybody's > purpose. There is no need to be so antagonistic at all times. > > > "The new code bootstraps .616% faster with a 99% confidence of > > being faster." > > > > 99% confidence on what basis? What's your sample size? > > Perhaps Lawrence can explain a bit more how he's getting these > numbers. But they are not pulled out of thin air and he does go to > the extra effort of measuring them and computing the differences. The intent of the work is to compare the performance of the unmodified compiler and the compiler with my patch. For each compiler, I run the third stage of the boot with -ftime-report ten times. By running the third stage, I test two things. Stage two has all the benefits of any performance improvements that the restructuring can deliver to end users. But since it is compiling stage three, it is also accounting for any increased time that the new C++ code takes in the bootstrap. The end customer won't pay that cost, but it was a concern among GCC developers. By parsing the log files, I extract total CPU time for each run. So, I have two samples, each with ten data points. Each sample has a sampled mean and a variance, from which you can compute a confidence interval, in which the true mean is likely to be. You can then compare the two confidence intervals to determine the likely hood that one is better or worse than the other. So, in the statement "The new code bootstraps .616% faster with a 99% confidence of being faster", the last phrase says if we were to run that same experiment 100 times, we might get one case where the compiler was slower. For most purposes, a 95% confidence is sufficient for medical interventions. Compile-time isn't that important, so we could easily get by on 70% confidence. In any event, the sample size is only relevant to the extent that larger sample sizes yield more confidence. More consistent runs also yield more confidence. Algorithmic changes, which would yield larger difference, would also yield more confidence. Since I report the confidence, you don't need to worry about sample size or isolation from system contention, etc. All those issues would have affected confidence. > > Why does the patch need this kind of "marketing"? > > Because (a) we have always said that we want to make sure that > the C++ conversion provides useful benefits, and (b) there has > been so much negative pressure on our work, that we sometimes > try to find some benefit when reality may provide neutral results. Yes, in particular, there was some concern that the cost of compiling the templates used in the hash tables would increase the bootstrap time significantly. In these cases, I have shown that the benefit of using them exceeds the cost of compiling them. If no one cares about these time reports, then I will gladly stop spending the effort to make them. > > > I, for one, think that it's excellent that Lawrence is > > > writing these cleanup patches and measuring what impact they > > > have on performance. Bonus points that they are making the > > > compiler faster. Speed of the compiler *is* a scalability > > > issue, and it's one that GCC doesn't appear to have paid all > > > that much attention to over the years. > > > > I just don't believe the 0.5% numbers. > > Then ask. Don't mock, please. -- Lawrence Crowl
Re: Convert more non-GTY htab_t to hash_table.
Could you please review the patch at http://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg40961.html? On 10/3/12, Lawrence Crowl wrote: > Sorry, one more time with the right file contents. > > On 10/2/12, Richard Guenther wrote: >> You are changing a hashtable used by fold checking, did you test >> with fold checking enabled? > > One small thinko fixed. Patch passes tests. > >> The cfg.c, dse.c and hash-table.h parts are ok for trunk, I'll >> leave the rest to respective maintainers of the pieces of the >> compiler. > > +cc > java: tro...@redhat.com > c: r...@redhat.com > objc: mikest...@comcast.net > cp: ja...@redhat.com > > > > Change more non-GTY hash tables to use the new type-safe template hash > table. > Constify member function parameters that can be const. > Correct a couple of expressions in formerly uninstantiated templates. > > The new code is 0.362% faster in bootstrap, with a 99.5% confidence of > being faster. > > Tested on x86-64. > > Okay for trunk? > > > Index: gcc/java/ChangeLog > > 2012-10-01 Lawrence Crowl > > * Make-lang.in (JAVA_OBJS): Add dependence on hash-table.o. > (JCFDUMP_OBJS): Add dependence on hash-table.o. > (jcf-io.o): Add dependence on hash-table.h. > * jcf-io.c (memoized_class_lookups): Change to use type-safe hash table. > > Index: gcc/c/ChangeLog > > 2012-10-01 Lawrence Crowl > > * Make-lang.in (c-decl.o): Add dependence on hash-table.h. > * c-decl.c (detect_field_duplicates_hash): Change to new type-safe > hash table. > > Index: gcc/objc/ChangeLog > > 2012-10-01 Lawrence Crowl > > * Make-lang.in (OBJC_OBJS): Add dependence on hash-table.o. > (objc-act.o): Add dependence on hash-table.h. > * objc-act.c (objc_detect_field_duplicates): Change to new type-safe > hash table. > > Index: gcc/ChangeLog > > 2012-10-01 Lawrence Crowl > > * Makefile.in (fold-const.o): Add depencence on hash-table.h. > (dse.o): Likewise. > (cfg.o): Likewise. > * fold-const.c (fold_checksum_tree): Change to new type-safe hash table. > * (print_fold_checksum): Likewise. > * cfg.c (var bb_original): Likewise. > * (var bb_copy): Likewise. > * (var loop_copy): Likewise. > * hash-table.h (template hash_table): Constify parameters for find... > and remove_elt... member functions. > (hash_table::empty) Correct size expression. > (hash_table::clear_slot) Correct deleted entry assignment. > * dse.c (var rtx_group_table): Change to new type-safe hash table. > > Index: gcc/cp/ChangeLog > > 2012-10-01 Lawrence Crowl > > * Make-lang.in (class.o): Add dependence on hash-table.h. > (tree.o): Likewise. > (semantics.o): Likewise. > * class.c (fixed_type_or_null): Change to new type-safe hash table. > * tree.c (verify_stmt_tree): Likewise. > (verify_stmt_tree_r): Likewise. > * semantics.c (struct nrv_data): Likewise. > > > Index: gcc/java/Make-lang.in > === > --- gcc/java/Make-lang.in (revision 191941) > +++ gcc/java/Make-lang.in (working copy) > @@ -83,10 +83,10 @@ JAVA_OBJS = java/class.o java/decl.o jav >java/zextract.o java/jcf-io.o java/win32-host.o java/jcf-parse.o > java/mangle.o \ >java/mangle_name.o java/builtins.o java/resource.o \ >java/jcf-depend.o \ > - java/jcf-path.o java/boehm.o java/java-gimplify.o > + java/jcf-path.o java/boehm.o java/java-gimplify.o hash-table.o > > JCFDUMP_OBJS = java/jcf-dump.o java/jcf-io.o java/jcf-depend.o > java/jcf-path.o \ > - java/win32-host.o java/zextract.o ggc-none.o > + java/win32-host.o java/zextract.o ggc-none.o hash-table.o > > JVGENMAIN_OBJS = java/jvgenmain.o java/mangle_name.o > > @@ -326,7 +326,7 @@ java/java-gimplify.o: java/java-gimplify > # jcf-io.o needs $(ZLIBINC) added to cflags. > CFLAGS-java/jcf-io.o += $(ZLIBINC) > java/jcf-io.o: java/jcf-io.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \ > - $(JAVA_TREE_H) java/zipfile.h > + $(JAVA_TREE_H) java/zipfile.h $(HASH_TABLE_H) > > # jcf-path.o needs a -D. > CFLAGS-java/jcf-path.o += \ > Index: gcc/java/jcf-io.c > === > --- gcc/java/jcf-io.c (revision 191941) > +++ gcc/java/jcf-io.c (working copy) > @@ -31,7 +31,7 @@ The Free Software Foundation is independ > #include "jcf.h" > #include "tree.h" > #include "java-tree.h" > -#include "hashtab.h" > +#include "hash-table.h&
Re: Convert more non-GTY htab_t to hash_table.
On 10/5/12, Richard Guenther wrote:
> On Thu, 4 Oct 2012, Lawrence Crowl wrote:
>> On 10/4/12, Richard Guenther wrote:
>> > On Tue, 2 Oct 2012, Lawrence Crowl wrote:
>> >> On 10/2/12, Richard Guenther wrote:
>> >> > On Mon, 1 Oct 2012, Lawrence Crowl wrote:
>> >> > > Change more non-GTY hash tables to use the new type-safe
>> >> > > template hash table. Constify member function parameters that
>> >> > > can be const. Correct a couple of expressions in formerly
>> >> > > uninstantiated templates.
>> >> > >
>> >> > > The new code is 0.362% faster in bootstrap, with a 99.5%
>> >> > > confidence of being faster.
>> >> > >
>> >> > > Tested on x86-64.
>> >> > >
>> >> > > Okay for trunk?
>> >> >
>> >> > You are changing a hashtable used by fold checking, did you test
>> >> > with fold checking enabled?
>> >>
>> >> I didn't know I had to do anything beyond the normal make check.
>> >> What do I do?
>> >>
>> >> > +/* Data structures used to maintain mapping between basic blocks
>> >> > and
>> >> > + copies. */
>> >> > +static hash_table bb_original;
>> >> > +static hash_table bb_copy;
>> >> >
>> >> > note that because hash_table has a constructor we now get global
>> >> > CTORs for all statics :( (and mx-protected local inits ...)
>> >>
>> >> The overhead for the global constructors isn't significant.
>> >> Only the function-local statics have mx-protection, and that can
>> >> be eliminated by making them global static.
>> >>
>> >> > Can you please try to remove the constructor from hash_table to
>> >> > avoid this overhead? (as a followup - that is, don't initialize
>> >> > htab)
>> >>
>> >> The initialization avoids potential errors in calling dispose.
>> >> I can do it, but I don't think the overhead (after moving the
>> >> function-local statics to global) will matter, and so I prefer to
>> >> keep the safety. So is the move of the statics sufficient or do
>> >> you still want to remove constructors?
>> >
>> > Hm, having them in-scope where they are used is good style.
>> > Why can't they be statically initialized and put in .data?
>> > Please make it so - you know C++ enough (ISTR value-initialization
>> > is default - which means NULL for the pointer?)
>>
>> Zero initialization is default for static variables, but not for
>> local or heap variables. We can live with the uninitialized memory
>
> Not for local static variables?
I intended my statement to include local static within static.
> I mean, isn't it possible to have an initializer like
>
> foo ()
> {
> static X x = X();
Without a default constructor definition, you would get zero
initialization by writing simply
static X x;
> that behaves in the way that it is "inlined", that is, implemented
> as putting x into .data, with proper initial contents?
The static initialization is permitted, but not required.
My general thinking is that if the dynamic initialization is a
problem, then we should change the compiler to do the permitted
inlining to static initialization.
BTW, C++11 addresses this issue head on, buy allowing you to declare
a constructor as 'constexpr', in which case static initialization
is guaranteed.
> Then, why isn't there a way that X is default (aka zero)
> initialized when there is no initializer? I simply want C behavior
> back here ...
>
> and I would have expected that not providing the
> hash_table::hash_table() constructor would just do that (with
> the complication that new-style allocation will end up with an
> uninitialized object, which we could avoid with providing a operator new
> implementation?).
Providing an operator new to do this task would be stretching the
limits of what programmers expect here. It's not clear it would
matter either, because the constructor is going to be the same cost
inside operator new and outside operator new.
The real issue is non-static local variables. Programmers using
htab_t had to remember to assign null to it.
void func (...)
{
htab_t foo = NULL;
If you forgot, bad things could happen.
The C++ equivalent would be, as you suggest above, but with
a longer name.
hash_table > x
= hash_table > x ();
I'm guessing there would be some objection to typing that.
The alternative is an init member function.
hash_table > x;
x.init ();
With the constructor, you don't have to remember and you don't
have to type more. If you have a variable, you know that it is
properly initialized.
>> in some cases, and add another function to explicitly null the
>> member in the rest of the cases. I am not convinced that extra
>> coding is worth the performance difference, particularly as I do
>> not expect that difference to be measureable.
>>
>> However we decide here, I think that work should be a separate patch,
>> as it will certainly touch more files than the current patch. So,
>> can we separate the issue?
>
> Sure, see my initial request.
--
Lawrence Crowl
Re: Convert more non-GTY htab_t to hash_table.
On 10/4/12, Richard Guenther wrote: > On Tue, 2 Oct 2012, Lawrence Crowl wrote: >> On 10/2/12, Richard Guenther wrote: >> > On Mon, 1 Oct 2012, Lawrence Crowl wrote: >> > > Change more non-GTY hash tables to use the new type-safe >> > > template hash table. Constify member function parameters that >> > > can be const. Correct a couple of expressions in formerly >> > > uninstantiated templates. >> > > >> > > The new code is 0.362% faster in bootstrap, with a 99.5% >> > > confidence of being faster. >> > > >> > > Tested on x86-64. >> > > >> > > Okay for trunk? >> > >> > You are changing a hashtable used by fold checking, did you test >> > with fold checking enabled? >> >> I didn't know I had to do anything beyond the normal make check. >> What do I do? >> >> > +/* Data structures used to maintain mapping between basic blocks and >> > + copies. */ >> > +static hash_table bb_original; >> > +static hash_table bb_copy; >> > >> > note that because hash_table has a constructor we now get global >> > CTORs for all statics :( (and mx-protected local inits ...) >> >> The overhead for the global constructors isn't significant. >> Only the function-local statics have mx-protection, and that can >> be eliminated by making them global static. >> >> > Can you please try to remove the constructor from hash_table to >> > avoid this overhead? (as a followup - that is, don't initialize >> > htab) >> >> The initialization avoids potential errors in calling dispose. >> I can do it, but I don't think the overhead (after moving the >> function-local statics to global) will matter, and so I prefer to >> keep the safety. So is the move of the statics sufficient or do >> you still want to remove constructors? > > Hm, having them in-scope where they are used is good style. > Why can't they be statically initialized and put in .data? > Please make it so - you know C++ enough (ISTR value-initialization > is default - which means NULL for the pointer?) Zero initialization is default for static variables, but not for local or heap variables. We can live with the uninitialized memory in some cases, and add another function to explicitly null the member in the rest of the cases. I am not convinced that extra coding is worth the performance difference, particularly as I do not expect that difference to be measureable. However we decide here, I think that work should be a separate patch, as it will certainly touch more files than the current patch. So, can we separate the issue? > > Richard. > >> > The cfg.c, dse.c and hash-table.h parts are ok for trunk, I'll >> > leave the rest to respective maintainers of the pieces of the >> > compiler. >> > >> > Thanks, >> > Richard. >> > >> >> >> >> Index: gcc/java/ChangeLog >> >> >> >> 2012-10-01 Lawrence Crowl >> >> >> >> * Make-lang.in (JAVA_OBJS): Add dependence on hash-table.o. >> >> (JCFDUMP_OBJS): Add dependence on hash-table.o. >> >> (jcf-io.o): Add dependence on hash-table.h. >> >> * jcf-io.c (memoized_class_lookups): Change to use type-safe hash >> >> table. >> >> >> >> Index: gcc/c/ChangeLog >> >> >> >> 2012-10-01 Lawrence Crowl >> >> >> >> * Make-lang.in (c-decl.o): Add dependence on hash-table.h. >> >> * c-decl.c (detect_field_duplicates_hash): Change to new type-safe >> >> hash table. >> >> >> >> Index: gcc/objc/ChangeLog >> >> >> >> 2012-10-01 Lawrence Crowl >> >> >> >> * Make-lang.in (OBJC_OBJS): Add dependence on hash-table.o. >> >> (objc-act.o): Add dependence on hash-table.h. >> >> * objc-act.c (objc_detect_field_duplicates): Change to new type-safe >> >> hash table. >> >> >> >> Index: gcc/ChangeLog >> >> >> >> 2012-10-01 Lawrence Crowl >> >> >> >> * Makefile.in (fold-const.o): Add depencence on hash-table.h. >> >> (dse.o): Likewise. >> >> (cfg.o): Likewise. >> >> * fold-const.c (fold_checksum_tree): Change to new type-safe hash >> >> table. >> >> * (print_fold_checksum): Likewise. >> >> * cfg.c (var bb_original): Likewise. >> >> * (var bb_copy): Likewise. >> >> * (va
Re: Convert more non-GTY htab_t to hash_table.
Sorry, one more time with the right file contents.
On 10/2/12, Richard Guenther wrote:
> You are changing a hashtable used by fold checking, did you test
> with fold checking enabled?
One small thinko fixed. Patch passes tests.
> The cfg.c, dse.c and hash-table.h parts are ok for trunk, I'll
> leave the rest to respective maintainers of the pieces of the
> compiler.
+cc
java: tro...@redhat.com
c: r...@redhat.com
objc: mikest...@comcast.net
cp: ja...@redhat.com
Change more non-GTY hash tables to use the new type-safe template hash table.
Constify member function parameters that can be const.
Correct a couple of expressions in formerly uninstantiated templates.
The new code is 0.362% faster in bootstrap, with a 99.5% confidence of
being faster.
Tested on x86-64.
Okay for trunk?
Index: gcc/java/ChangeLog
2012-10-01 Lawrence Crowl
* Make-lang.in (JAVA_OBJS): Add dependence on hash-table.o.
(JCFDUMP_OBJS): Add dependence on hash-table.o.
(jcf-io.o): Add dependence on hash-table.h.
* jcf-io.c (memoized_class_lookups): Change to use type-safe hash table.
Index: gcc/c/ChangeLog
2012-10-01 Lawrence Crowl
* Make-lang.in (c-decl.o): Add dependence on hash-table.h.
* c-decl.c (detect_field_duplicates_hash): Change to new type-safe
hash table.
Index: gcc/objc/ChangeLog
2012-10-01 Lawrence Crowl
* Make-lang.in (OBJC_OBJS): Add dependence on hash-table.o.
(objc-act.o): Add dependence on hash-table.h.
* objc-act.c (objc_detect_field_duplicates): Change to new type-safe
hash table.
Index: gcc/ChangeLog
2012-10-01 Lawrence Crowl
* Makefile.in (fold-const.o): Add depencence on hash-table.h.
(dse.o): Likewise.
(cfg.o): Likewise.
* fold-const.c (fold_checksum_tree): Change to new type-safe hash table.
* (print_fold_checksum): Likewise.
* cfg.c (var bb_original): Likewise.
* (var bb_copy): Likewise.
* (var loop_copy): Likewise.
* hash-table.h (template hash_table): Constify parameters for find...
and remove_elt... member functions.
(hash_table::empty) Correct size expression.
(hash_table::clear_slot) Correct deleted entry assignment.
* dse.c (var rtx_group_table): Change to new type-safe hash table.
Index: gcc/cp/ChangeLog
2012-10-01 Lawrence Crowl
* Make-lang.in (class.o): Add dependence on hash-table.h.
(tree.o): Likewise.
(semantics.o): Likewise.
* class.c (fixed_type_or_null): Change to new type-safe hash table.
* tree.c (verify_stmt_tree): Likewise.
(verify_stmt_tree_r): Likewise.
* semantics.c (struct nrv_data): Likewise.
Index: gcc/java/Make-lang.in
===
--- gcc/java/Make-lang.in (revision 191941)
+++ gcc/java/Make-lang.in (working copy)
@@ -83,10 +83,10 @@ JAVA_OBJS = java/class.o java/decl.o jav
java/zextract.o java/jcf-io.o java/win32-host.o java/jcf-parse.o
java/mangle.o \
java/mangle_name.o java/builtins.o java/resource.o \
java/jcf-depend.o \
- java/jcf-path.o java/boehm.o java/java-gimplify.o
+ java/jcf-path.o java/boehm.o java/java-gimplify.o hash-table.o
JCFDUMP_OBJS = java/jcf-dump.o java/jcf-io.o java/jcf-depend.o
java/jcf-path.o \
- java/win32-host.o java/zextract.o ggc-none.o
+ java/win32-host.o java/zextract.o ggc-none.o hash-table.o
JVGENMAIN_OBJS = java/jvgenmain.o java/mangle_name.o
@@ -326,7 +326,7 @@ java/java-gimplify.o: java/java-gimplify
# jcf-io.o needs $(ZLIBINC) added to cflags.
CFLAGS-java/jcf-io.o += $(ZLIBINC)
java/jcf-io.o: java/jcf-io.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \
- $(JAVA_TREE_H) java/zipfile.h
+ $(JAVA_TREE_H) java/zipfile.h $(HASH_TABLE_H)
# jcf-path.o needs a -D.
CFLAGS-java/jcf-path.o += \
Index: gcc/java/jcf-io.c
===
--- gcc/java/jcf-io.c (revision 191941)
+++ gcc/java/jcf-io.c (working copy)
@@ -31,7 +31,7 @@ The Free Software Foundation is independ
#include "jcf.h"
#include "tree.h"
#include "java-tree.h"
-#include "hashtab.h"
+#include "hash-table.h"
#include
#include "zlib.h"
@@ -271,20 +271,34 @@ find_classfile (char *filename, JCF *jcf
return open_class (filename, jcf, fd, dep_name);
}
-/* Returns 1 if the CLASSNAME (really a char *) matches the name
- stored in TABLE_ENTRY (also a char *). */
-static int
-memoized_class_lookup_eq (const void *table_entry, const void *classname)
+/* Hash table helper. */
+
+struct charstar_hash : typed_noop_remove
{
- return strcmp ((const char *)classname, (const char *)table_entry) == 0;
+ typedef const char T;
+ static inline hashval_t hash (const T *candidate);
+ static inline bool equal (const T *existing, const T *candidate);
+};
+
+inline h
Re: Convert more non-GTY htab_t to hash_table.
On 10/2/12, Richard Guenther wrote: > On Mon, 1 Oct 2012, Lawrence Crowl wrote: > >> Change more non-GTY hash tables to use the new type-safe template hash >> table. >> Constify member function parameters that can be const. >> Correct a couple of expressions in formerly uninstantiated templates. >> >> The new code is 0.362% faster in bootstrap, with a 99.5% confidence of >> being faster. >> >> Tested on x86-64. >> >> Okay for trunk? > > You are changing a hashtable used by fold checking, did you test > with fold checking enabled? > > +/* Data structures used to maintain mapping between basic blocks and > + copies. */ > +static hash_table bb_original; > +static hash_table bb_copy; > > note that because hash_table has a constructor we now get global > CTORs for all statics :( (and mx-protected local inits ...) > Can you please try to remove the constructor from hash_table to > avoid this overhead? (as a followup - that is, don't initialize htab) > > The cfg.c, dse.c and hash-table.h parts are ok for trunk, I'll leave the > rest to > respective maintainers of the pieces of the compiler. > > Thanks, > Richard. > >> >> Index: gcc/java/ChangeLog >> >> 2012-10-01 Lawrence Crowl >> >> * Make-lang.in (JAVA_OBJS): Add dependence on hash-table.o. >> (JCFDUMP_OBJS): Add dependence on hash-table.o. >> (jcf-io.o): Add dependence on hash-table.h. >> * jcf-io.c (memoized_class_lookups): Change to use type-safe hash table. >> >> Index: gcc/c/ChangeLog >> >> 2012-10-01 Lawrence Crowl >> >> * Make-lang.in (c-decl.o): Add dependence on hash-table.h. >> * c-decl.c (detect_field_duplicates_hash): Change to new type-safe >> hash table. >> >> Index: gcc/objc/ChangeLog >> >> 2012-10-01 Lawrence Crowl >> >> * Make-lang.in (OBJC_OBJS): Add dependence on hash-table.o. >> (objc-act.o): Add dependence on hash-table.h. >> * objc-act.c (objc_detect_field_duplicates): Change to new type-safe >> hash table. >> >> Index: gcc/ChangeLog >> >> 2012-10-01 Lawrence Crowl >> >> * Makefile.in (fold-const.o): Add depencence on hash-table.h. >> (dse.o): Likewise. >> (cfg.o): Likewise. >> * fold-const.c (fold_checksum_tree): Change to new type-safe hash table. >> * (print_fold_checksum): Likewise. >> * cfg.c (var bb_original): Likewise. >> * (var bb_copy): Likewise. >> * (var loop_copy): Likewise. >> * hash-table.h (template hash_table): Constify parameters for find... >> and remove_elt... member functions. >> (hash_table::empty) Correct size expression. >> (hash_table::clear_slot) Correct deleted entry assignment. >> * dse.c (var rtx_group_table): Change to new type-safe hash table. >> >> Index: gcc/cp/ChangeLog >> >> 2012-10-01 Lawrence Crowl >> >> * Make-lang.in (class.o): Add dependence on hash-table.h. >> (tree.o): Likewise. >> (semantics.o): Likewise. >> * class.c (fixed_type_or_null): Change to new type-safe hash table. >> * tree.c (verify_stmt_tree): Likewise. >> (verify_stmt_tree_r): Likewise. >> * semantics.c (struct nrv_data): Likewise. >> >> >> Index: gcc/java/Make-lang.in >> === >> --- gcc/java/Make-lang.in(revision 191941) >> +++ gcc/java/Make-lang.in(working copy) >> @@ -83,10 +83,10 @@ JAVA_OBJS = java/class.o java/decl.o jav >>java/zextract.o java/jcf-io.o java/win32-host.o java/jcf-parse.o >> java/mangle.o \ >>java/mangle_name.o java/builtins.o java/resource.o \ >>java/jcf-depend.o \ >> - java/jcf-path.o java/boehm.o java/java-gimplify.o >> + java/jcf-path.o java/boehm.o java/java-gimplify.o hash-table.o >> >> JCFDUMP_OBJS = java/jcf-dump.o java/jcf-io.o java/jcf-depend.o >> java/jcf-path.o \ >> -java/win32-host.o java/zextract.o ggc-none.o >> +java/win32-host.o java/zextract.o ggc-none.o hash-table.o >> >> JVGENMAIN_OBJS = java/jvgenmain.o java/mangle_name.o >> >> @@ -326,7 +326,7 @@ java/java-gimplify.o: java/java-gimplify >> # jcf-io.o needs $(ZLIBINC) added to cflags. >> CFLAGS-java/jcf-io.o += $(ZLIBINC) >> java/jcf-io.o: java/jcf-io.c $(CONFIG_H) $(SYSTEM_H) coretypes.h \ >> - $(JAVA_TREE_H) java/zipfile.h >> + $(JAVA_TREE_H) java/zipfile.h $(HASH_TABLE_H) >> >> # jcf-path.o needs a
Add myself to MAINTAINERS
Adding myself as write after approval. Index: MAINTAINERS === --- MAINTAINERS (revision 191941) +++ MAINTAINERS (working copy) @@ -342,6 +342,7 @@ R. Kelley Cook kc...@gcc.gnu.org Christian Cornelssen cc...@cs.tu-berlin.de François-Xavier Coudertfxcoud...@gcc.gnu.org Cary Coutant ccout...@google.com +Lawrence Crowl cr...@google.com Ian Dall i...@beware.dropbear.id.au David Daneydavid.da...@caviumnetworks.com Bud Davis jmda...@link.com -- Lawrence Crowl
Re: Convert more non-GTY htab_t to hash_table.
On 10/2/12, Ian Lance Taylor wrote: > On Oct 2, 2012 Lawrence Crowl wrote: > > On 10/2/12, Richard Guenther wrote: > > > You are changing a hashtable used by fold checking, did you > > > test with fold checking enabled? > > > > I didn't know I had to do anything beyond the normal make check. > > What do I do? > > Fold checking is not enabled by default because of high overhead > and general pointlessness. To enable it, when you run configure, > use --enable-checking=yes,fold. So, why have the feature if it is pointless? Just curious. -- Lawrence Crowl
