Re: [patch] Apply conditional down cast to cgraph.h et.al.

[Lawrence Crowl]

On 10/30/12, Diego Novillo dnovi...@google.com wrote:
 On 2012-10-29 15:01 , Lawrence Crowl wrote:
 On 10/27/12, Marc Glisse marc.gli...@inria.fr wrote:
 On Fri, 26 Oct 2012, Lawrence Crowl wrote:
 2012-10-26  Lawrence Crowl  cr...@google.com

 missing ''

 Fixed.

* is-a.h: New.
(is_a T (U*)): New.  Test for is-a relationship.
(as_a T (U*)): New.  Treat as a derived type.
(dyn_cast T (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 TYPE (pointer)
Tests whether the pointer actually points to a more derived TYPE.

 TYPE *as_a TYPE (pointer)
Converts pointer to a TYPE*.

 TYPE *dyn_cast TYPE (pointer)
Converts pointer to TYPE* if and only if is_a TYPE 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 cgraph_node (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 varpool_node (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 cgraph_node and is_a varpool_node.  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  cr...@google.com

  * is-a.h: New.
  (is_a T (U*)): New.  Test for is-a relationship.
  (as_a T (U*)): New.  Treat as a derived type.
  (dyn_cast T (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 cgraph_node::test (symtab_node_def *)): New.
  (is_a_helper varpool_node::test (symtab_node_def *)): New.
  (symtab_node_def::try_function): New.  Change most calls to
  symtab_function_p with calls to dyn_cast cgraph_node (p).
  (symtab_node_def::try_variable): New.  Change most calls to
  symtab_variable_p with calls to dyn_cast varpool_node (p).
  (symtab_function_p): Remove.  Change callers to use
  is_a cgraph_node (p) instead.
  (symtab_variable_p): Remove.  Change callers to use
  is_a varpool_node (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 few questions and
 comments below.  Honza, the patch looks OK to me but it touches a bunch
 of cgraph code, could you please go over it?


 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

Re: [patch] Apply conditional down cast to cgraph.h et.al.

[Lawrence Crowl]

On 10/30/12, Diego Novillo dnovi...@google.com wrote:
 On Tue, Oct 30, 2012 at 4:53 PM, Lawrence Crowl cr...@googlers.com wrote:
 On 10/30/12, Diego Novillo dnovi...@google.com 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 cgraph_node (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: GCC 4.8.0 Status Report (2012-10-29), Stage 1 to end soon

[Lawrence Crowl]

On 10/30/12, Diego Novillo dnovi...@google.com wrote:
 On Mon, Oct 29, 2012 at 1:56 PM, Jakub Jelinek ja...@redhat.com 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 aol...@redhat.com 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

[patch] Normalize bitmap iteration.

[Lawrence Crowl]

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  cr...@google.com

* 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

[patch] Remove unused ebitmap and unused sbitmap functions.

[Lawrence Crowl]

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  cr...@google.com

* 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/sbitmap.h
===
--- gcc/sbitmap.h   (revision 193049)
+++ gcc/sbitmap.h   (working copy)
@@ -42,11 +42,10 @@ along with GCC

Re: [patch] Normalize more bitmap function names.

[Lawrence Crowl]

On 11/1/12, Diego Novillo dnovi...@google.com 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

Re: [patch] Normalize bitmap iteration.

[Lawrence Crowl]

On 11/1/12, Diego Novillo dnovi...@google.com 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] Remove unused ebitmap and unused sbitmap functions.

[Lawrence Crowl]

On 11/1/12, Diego Novillo dnovi...@google.com wrote:
 On 2012-10-31 18:47 , Lawrence Crowl wrote:
 2012-10-31  Lawrence Crowl  cr...@google.com

  * 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] Apply conditional down cast to cgraph.h et.al.

[Lawrence Crowl]

On 11/2/12, Eric Botcazou ebotca...@adacore.com wrote:
 Index: gcc/ChangeLog

 2012-10-31  Lawrence Crowl  cr...@google.com

  * is-a.h: New.
  (is_a T (U*)): New.  Test for is-a relationship.
  (as_a T (U*)): New.  Treat as a derived type.
  (dyn_cast T (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 cgraph_node::test (symtab_node_def *)): New.
  (is_a_helper varpool_node::test (symtab_node_def *)): New.
  (symtab_node_def::try_function): New.  Change most calls to
  symtab_function_p with calls to dyn_cast cgraph_node (p).
  (symtab_node_def::try_variable): New.  Change most calls to
  symtab_variable_p with calls to dyn_cast varpool_node (p).
  (symtab_function_p): Remove.  Change callers to use
 is_a cgraph_node (p) instead.
  (symtab_variable_p): Remove.  Change callers to use
 is_a varpool_node (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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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

[patch] Contribute performance comparison script.

[Lawrence Crowl]

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  cr...@google.com

* 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 cr...@google.com
+#
+# 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 directory,
+#  matching a Unix shell file pattern.
+#  Returns an iterator over the file names.
+#  
+#  for path, dirlist, filelist in os.walk(topdir):
+#for name in fnmatch.filter(filelist, filepat

Re: [patch] Contribute performance comparison script.

[Lawrence Crowl]

On 11/6/12, Diego Novillo dnovi...@google.com wrote:
 On Nov 5, 2012 Lawrence Crowl cr...@googlers.com wrote:
 
  2012-11-05  Lawrence Crowl  cr...@google.com
 
   * compare_two_ftime_report_sets: New.

 OK.  Thanks.

Committed.

-- 
Lawrence Crowl

[cxx-conversion] LTO-related hash tables

[Lawrence Crowl]

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  cr...@google.com

* 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 freeing_string_slot_hasher 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_unpack_string (struct dat
 unsigned HOST_WIDE_INT

[cxx-conversion] graphite-related hash tables

[Lawrence Crowl]

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  cr...@google.com

* graphite-htab.h: New.
(typedef hash_table bb_pbb_hasher 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 sebastian@amd.com.
+
+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 bb_pbb_def
+{
+  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_hasher bb_pbb_htab_type;
+
+extern bool gloog (scop_p, bb_pbb_htab_type);
+poly_bb_p find_pbb_via_hash (bb_pbb_htab_type

[cxx-conversion] gimplify_ctx::temp_htab hash table

[Lawrence Crowl]

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  cr...@google.com

* 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_formal);
- *slot = (void *) elt_p;
+ *slot = elt_p;
}
   else
{
- elt_p = (elt_t *) *slot;
+ elt_p = *slot;
   ret = elt_p-temp;
}
 }
Index: gcc/gimple-fold.c

[cxx-conversion] tree-related hash tables

[Lawrence Crowl]

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  cr...@google.com

* 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 veccomplex_lattice_t 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 vectree 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 dnovi...@redhat.com
+
+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 int_tree_map
+{
+  typedef int_tree_map value_type;
+  typedef int_tree_map compare_type;
+  static inline hashval_t hash (const

[cxx-conversion] ggc-common hash tables

[Lawrence Crowl]

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  cr...@google.com

* 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 typename Argument,
int (*Callback) (value_type **slot, Argument argument)
@@ -431,9 +431,9 @@ hash_table Descriptor, Allocator::is_c
 template typename Descriptor,
  template typename Type class Allocator
 inline typename Descriptor::value_type *
-hash_table Descriptor, Allocator::find (const compare_type *comparable)
+hash_table Descriptor, Allocator::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 typename Descriptor,
  template typename Type class Allocator
 inline typename Descriptor::value_type **
 hash_table Descriptor, Allocator
-::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 Descriptor, Allocator
 template typename Descriptor,
  template typename Type class Allocator
 inline void
-hash_table Descriptor, Allocator::remove_elt (const compare_type *comparable)
+hash_table Descriptor, Allocator::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 Descriptor, Allocator::size
 template typename Descriptor,
  template typename Type class Allocator
 inline size_t
-hash_table Descriptor, Allocator::elements()
+hash_table Descriptor, Allocator::elements ()
 {
   return htab-n_elements - htab-n_deleted;
 }


+/* Return the current number of elements in this hash table. */
+
+template typename Descriptor,
+ template typename Type class Allocator
+inline size_t
+hash_table Descriptor, Allocator::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

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/18/12, Eric Botcazou ebotca...@adacore.com wrote:
 When, the property test is embedded within a larger condition, a little
 restructuring is required to pull out the secondary conditions.  For
 example,

   if (symtab_variable_p (node)
varpool (node)-finalized)
 varpool_analyze_node (varpool (node));

 becomes

   if (varpool_node *vnode = node-try_variable ())
 if (vnode-finalized)
   varpool_analyze_node (vnode);

 Please avoid cascading if's like this, use the existing  idiom instead.

The language syntax would bind the conditional into the intializer, as in

  if (varpool_node *vnode = (node-try_variable ()
  vnode-finalized))
varpool_analyze_node (vnode);

which does not type-match.

So, if you want the type saftey and performance, the cascade is really
unavoidable.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/18/12, Richard Guenther richard.guent...@gmail.com wrote:
 On Sep 18, 2012 Lawrence Crowl cr...@googlers.com wrote:
  * cgraph.h (varpool_node): Rename to varpool_node_for_tree.

 Sure it should be varpool_node_for_decl, if any.
 Or varpool_node_from_decl (grep for what is more common)

Grep says _for_decl wins over _from_decl by 113 to 74.
I will make that change unless I hear an objection soon.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/19/12, Eric Botcazou ebotca...@adacore.com wrote:
  The language syntax would bind the conditional into the
  intializer, as in
 
if (varpool_node *vnode = (node-try_variable ()
vnode-finalized))
  varpool_analyze_node (vnode);
 
  which does not type-match.
 
  So, if you want the type saftey and performance, the cascade
  is really unavoidable.

 Just write:

   varpool_node *vnode;

   if ((vnode = node-try_variable ())  vnode-finalized)
 varpool_analyze_node (vnode);

 This has been the standard style for the past 2 decades and
 trading it for cascading if's is really a bad idea.

Assignments in if statements are known to cause confusion.

The point of the change is to limit the scope of the variable
to the if statement, which prevents its unintended use later.
It acts like a type switch.

Why do you think cascading ifs is a bad idea?

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/19/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sep 19, 2012 Richard Guenther richard.guent...@gmail.com wrote:
  Indeed.  Btw, can we not provide a specialization for
  dynamic_cast ?  This -try_... looks awkward to me compared
  to the more familiar
 
  vnode = dynamic_cast varpool_node (node)
 
  but yeah - dynamic_cast is not a template ... (but maybe there
  is some standard library piece that mimics it?).

 No, it is a language primitive.

 but we can define out own operation with similar syntax that allows
 for specialization, whose generic implementation uses dynamic_cast.

   templatetypename T, typename U
   T* is(U* u) {
   return dynamic_castT*(u);
   }

At this point, dynamic_cast is not available because we do not
yet have polymorphic types.  There has been some resistance to
that notion.

Absent dynamic cast, we need to specialize for various type
combinations.  Function template specialization would be handy,
but C++ does not directly support that.  We could work around
that.  However, in the end, the fact that try_whatever is a member
function means that we can use a notation that depends on context
and so can be shorter.  That is, we can write 'function' instead of
'cgraph_node *'.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/19/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Wed, Sep 19, 2012 at 1:39 PM, Lawrence Crowl cr...@googlers.com wrote:
 On 9/19/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sep 19, 2012 Richard Guenther richard.guent...@gmail.com wrote:
  Indeed.  Btw, can we not provide a specialization for
  dynamic_cast ?  This -try_... looks awkward to me compared
  to the more familiar
 
  vnode = dynamic_cast varpool_node (node)
 
  but yeah - dynamic_cast is not a template ... (but maybe there
  is some standard library piece that mimics it?).

 No, it is a language primitive.

 but we can define out own operation with similar syntax that allows
 for specialization, whose generic implementation uses dynamic_cast.

   templatetypename T, typename U
   T* is(U* u) {
   return dynamic_castT*(u);
   }

 At this point, dynamic_cast is not available because we do not
 yet have polymorphic types.  There has been some resistance to
 that notion.

 Hmm, when did we rule that out?

We have not ruled it out, but folks are, rightly, concerned about any
size increase in critical data structures.  We are also currently
lacking a gengtype that will handle inheritance.  So, for now at
least, we need a scheme that will work across both inheritance and
our current tag/union approach.

 We currently implement dynamic_cast using the poor man's simulation
 based on tree_code checking.  We can just as well give such
 simulation the is notation.

 Absent dynamic cast, we need to specialize for various type
 combinations.  Function template specialization would be handy,
 but C++ does not directly support that.  We could work around
 that.

 We can always use the standard workaround: call a static member
 function of a class template that can be specialized at will.

 However, in the end, the fact that try_whatever is a member
 function means that we can use a notation that depends on context
 and so can be shorter.  That is, we can write 'function' instead of
 'cgraph_node *'.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/20/12, Michael Matz m...@suse.de wrote:
 On Wed, 19 Sep 2012, Lawrence Crowl wrote:
  On 9/19/12, Eric Botcazou ebotca...@adacore.com wrote:
The language syntax would bind the conditional into the
intializer, as in
   
  if (varpool_node *vnode = (node-try_variable ()
  vnode-finalized))
varpool_analyze_node (vnode);
   
which does not type-match.
   
So, if you want the type saftey and performance, the cascade
is really unavoidable.
  
   Just write:
  
 varpool_node *vnode;
  
 if ((vnode = node-try_variable ())  vnode-finalized)
   varpool_analyze_node (vnode);
  
   This has been the standard style for the past 2 decades and
   trading it for cascading if's is really a bad idea.
 
  Assignments in if statements are known to cause confusion.

 So?  Make it:

   varpool_node *vnode = node-try_variable ();
   if (vnode  vnode-finalized)
 varpool_analyze_node (vnode);

  The point of the change is to limit the scope of the variable
  to the if statement, which prevents its unintended use later.

 I'm not worried about this.

It is helpful to have the language and the usage in concurrence.

Okay, so unless someone objects, I'll move the variable out when
it introduces a cacade.

  Why do you think cascading ifs is a bad idea?

 Precedent.  Confusion in case of dangling else (requiring more {},
 and hence even more indentation).  Ugly.

I generally take ugly as an indication that the function needs
refactoring.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 9/20/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Sep 20, 2012 Lawrence Crowl cr...@googlers.com wrote:
Why do you think cascading ifs is a bad idea?
  
   Precedent.  Confusion in case of dangling else (requiring
   more {}, and hence even more indentation).  Ugly.
 
  I generally take ugly as an indication that the function needs
  refactoring.

 Hear!  Hear! Hear!

 Sadly, many functions in GCC (at least in cp/) are in that
 category :-(

It is not just GCC.  Most large codebases I've seen have had lots of
large functions.  It takes persistent effort to beat back entropy.

I once chatted with a guy who worked on a project with a strict
requirement that every function fit on one screen, and that was
when screens were 24 lines.  He said their bug rate was really low.
And the one function they made an exception for turned out the have
the worst bug in it.

-- 
Lawrence Crowl

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

Add functions symtab_node_def::try_function and symtab_node_def::try_variable.
These function return a pointer to the more specific type (e.g. cgraph_node*)
if and only if the general type (symtab_node aka symtab_node_def*) is an
instance of the specific type.  These functions are essentially checked down
casts.

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 = node-try_function ())
{
  
}

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 inappropritately.)

  if (symtab_variable_p (node)
   varpool (node)-finalized)
varpool_analyze_node (varpool (node));

becomes

  varpool_node *vnode = node-try_variable ();
  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 member functions symtab_node_def::is_function and
symtab_node_def::is_variable.  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_tree.


The new code bootstraps .616% faster with a 99% confidence of being faster.


Tested on x86_64.


Okay for trunk?



Index: gcc/ChangeLog

2012-09-18  Lawrence Crowl  cr...@google.com

* cgraph.h (varpool_node): Rename to varpool_node_for_tree.
Adjust callers to match.
(symtab_node_def::try_function): New.
Change most calls to symtab_function_p with calls to
symtab_node_def::try_function.
(symtab_node_def::try_variable): New.
Change most calls to symtab_variable_p with calls to
symtab_node_def::try_variable.
(symtab_function_p): Rename to symtab_node_def::is_function.
Adjust remaining callers to match.
(symtab_variable_p): Rename to symtab_node_def::is_variable.
Adjust remaining callers to match.
* cgraph.c (cgraph_node_for_asm): Remove redundant call to
symtab_node_for_asm.
* graphunit.c (symbol_finalized_and_needed): New.
(symbol_finalized): New.
(cgraph_analyze_functions): Split complicated conditionals out into
above new functions.



Index: gcc/lto-symtab.c
===
--- gcc/lto-symtab.c(revision 191403)
+++ gcc/lto-symtab.c(working copy)
@@ -743,7 +743,7 @@ lto_symtab_merge_cgraph_nodes_1 (void **
{
  if (!prevailing-vnode)
{
- prevailing-vnode = varpool_node (prevailing-decl);
+ prevailing-vnode = varpool_node_for_tree (prevailing-decl);
  prevailing-vnode-alias = true;
}
  lto_varpool_replace_node (e-vnode, prevailing-vnode);
Index: gcc/cgraphbuild.c
===
--- gcc/cgraphbuild.c   (revision 191403)
+++ 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_tree (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);
+ struct varpool_node *vnode = varpool_node_for_tree (type);
  ipa_record_reference ((symtab_node)node,
(symtab_node)vnode,
IPA_REF_ADDR, NULL);
@@ -233,7 +233,7 @@ mark_address (gimple stmt, tree addr, vo
   else if (addr  TREE_CODE (addr) == VAR_DECL
(TREE_STATIC (addr) || DECL_EXTERNAL (addr)))
 {
-  struct varpool_node *vnode = varpool_node (addr);
+  struct varpool_node *vnode = varpool_node_for_tree (addr);

   ipa_record_reference ((symtab_node

Re: Merge C++ conversion into trunk (4/6 - hash table rewrite)

[Lawrence Crowl]

On 8/15/12, Richard Henderson r...@redhat.com wrote:
 On 2012-08-15 07:29, Richard Guenther wrote:
  +   typedef typename Element::Element_t Element_t;

 Can we use something less ugly than Element_t?
 Such as

   typedef typename Element::T T;

 ?  Given that this name is scoped anyway...

I've been finding the use of T as a typedef confusing.  It sort of
flies in the face of all existing convention.  The C++ standard would
use either element_type or value_type.  I suggest a rename, but I'm
guessing that folks don't want something as verbose as element_type.
How about elemtype?  Any objections to me changing it to that?

-- 
Lawrence Crowl

Re: Merge C++ conversion into trunk (4/6 - hash table rewrite)

[Lawrence Crowl]

On 9/26/12, Michael Matz m...@suse.de wrote:
 On Tue, 25 Sep 2012, Lawrence Crowl wrote:
  On 8/15/12, Richard Henderson r...@redhat.com wrote:
   On 2012-08-15 07:29, Richard Guenther wrote:
typedef typename Element::Element_t Element_t;
  
   Can we use something less ugly than Element_t?
   Such as typedef typename Element::T T;
   ?  Given that this name is scoped anyway...
 
  I've been finding the use of T as a typedef confusing.

 Why?  As type placeholder in templates it's quite customary,
 as the most interesting to users of this template.  I would
 have no trouble at all to see declarations like T x = getme();
 in a function.  In some way I even prefer that to some lower-case
 variant, because it reminds me that this specific T is actually
 variant.

The problem is that while T is customary as a template parameter, I
have never seen it used as a typedef name.  And that's the situation
that we are in now.

 A lower-case type name indicates to me a non-changing type,
 i.e. nothing that depends on a template.  In C we only had
 such types so we used lower-case names everywhere.  With C++
 and templates I think we should start using upper case for some
 very specific use cases, like first letter of dependend types.

How would you distinguish them from template parameter names,
which by convention have an upper case first letter?

What about non-type dependent names?

I think we really do need a separate convention for the two,
because dependent members of class templates often need special
access syntax.

  It sort of flies in the face of all existing convention.

 If you talk about the conventions used for the c++ standard
 library, then they are IMHO quite ugly and we should not follow
 them.

The advantage to following them is that they will surprise no one.
Do you have an alternate suggestion, one that does not confuse
template parameters and dependent names?

-- 
Lawrence Crowl

Re: Merge C++ conversion into trunk (4/6 - hash table rewrite)

[Lawrence Crowl]

On 9/27/12, Michael Matz m...@suse.de wrote:
 On Wed, 26 Sep 2012, Lawrence Crowl wrote:
   A lower-case type name indicates to me a non-changing type,
   i.e. nothing that depends on a template.  In C we only had
   such types so we used lower-case names everywhere.  With C++
   and templates I think we should start using upper case for some
   very specific use cases, like first letter of dependend types.
 
  How would you distinguish them from template parameter names,
  which by convention have an upper case first letter?

 I wouldn't.  If the distinction becomes so important that authors
 need to see the speciality immediately by having a different
 convention how to spell names, then I think we did something wrong,
 and we should simplify the code.

  What about non-type dependent names?

 I'm not sure what you're asking.  Let's make an example:

 template typename T
 struct D : BT
 {
   typedef typename BT::E E; // element_type
   E getme (int index);
 }

Inside that struct, lets say we have a field of type E.  Do we name
it F or f?

 In fact, as BT::E would probably be defined like typedef
 typename T E, I would even have no issue to call the above E
 also T.  The distinction between the template arg name and the
 typedef would be blurred, and I say, so what; one is a typedef
 of the other and hence mostly equivalent for practical purposes.
 (And if they aren't, then again, we did something too complicated
 with the switch to C++).

  The advantage to following them is that they will surprise
  no one.

 They will surprise everyone used to different conventions, for
 instance Qt, so that's not a reason.

Anyone using the standard library will not be surprised if we follow
the conventions of the standard library.  I'd guess that the number
standard library programmers outnumbers the Qt programmers by 100
to 1.  I'd guess that the number of Qt programmers that do not know
the standard library is a minority.

  Do you have an alternate suggestion, one that does not confuse
  template parameters and dependent names?

 Upper last character?  Just kidding :)  Too many detailed rules
 for conventions are the death of them, use rules of thumbs,
 my one would be somehow depends on template args - has upper
 character in name, where somehow depends on includes is a.

Ah, but there is a problem.  That typedef name does not necessarily
depend on a template parameter.

It is common practice to have

struct Q
{
  typedef int E;
  E getme (int index);
};

and use it in exactly the same places you would use Dsomething.

In fact, one place is in the hash table code we are discussing.
The hash descriptor type may not itself be a template.  I believe
that few of them will actually be templates.

So, if E implies comes from template, the implication is wrong.

If we were to follow C++ standard library conventions, we would call
it value_type.  That would be my preference.  However, if folks
want a shorter name, I'll live with that too.  But as it stands,
the current name is very confusing.

-- 
Lawrence Crowl

Re: Merge C++ conversion into trunk (4/6 - hash table rewrite)

[Lawrence Crowl]

On 9/28/12, Michael Matz m...@suse.de wrote:
 On Thu, 27 Sep 2012, Lawrence Crowl wrote:
   template typename T
   struct D : BT
   {
 typedef typename BT::E E; // element_type
 E getme (int index);
   }
 
  Inside that struct, lets say we have a field of type E.  Do we name
  it F or f?

 IMHO only for types, not for any other decls.

Do you have an alternate suggestion, one that does not confuse
template parameters and dependent names?
  
   Upper last character?  Just kidding :)  Too many detailed rules
   for conventions are the death of them, use rules of thumbs,
   my one would be somehow depends on template args - has upper
   character in name, where somehow depends on includes is a.
 
  Ah, but there is a problem.  That typedef name does not necessarily
  depend on a template parameter.
 
  It is common practice to have
 
  struct Q
  {
typedef int E;
E getme (int index);
  };

 Easy: I wouldn't make a typedef for Q::E that merely is int.  The reason
 is that it makes knowing what getme really returns harder.  You have to
 look it up always (or know the class already).  In fact that's one of my
 gripes with the standard library, much too much indirection through
 entities merely referring to other entities.  Might be only important for
 the libraries implementors but I sure hope that we don't start down that
 road in GCC.

  In fact, one place is in the hash table code we are discussing.
  The hash descriptor type may not itself be a template.  I believe
  that few of them will actually be templates.

 Then I don't see the need for class-local typedefs.

  So, if E implies comes from template, the implication is wrong.
 
  If we were to follow C++ standard library conventions, we would call it
  value_type.

 Well, but value_type surely does depend on the hashtables
 type argument, doesn't it?  After all it is a typedef from
 'Key'.  I would expect that htabtree::value_type is tree, and
 htabint::value_type is int, and I would like to see it named
 htabtree::T or ::E.

One declares a hash table as follows.

  hash_table hash_descriptor variable;

The type stored in the hash table is not part of the declaration.
It is part of the descriptor, along with other things like the
hash function.  The hash table essentially queries the descriptor
for the value type.  For example,

  template Descriptor class hash_table {
typename Descriptor::value_type *storage;
...

More typically though, the typedef is repeated inside the class to
avoid excess verbosity, and more importantly, to reexport the name.

  template Descriptor class hash_table {
typedef typename Descriptor::value_type value_type;
value_type *storage;
...

Using these typedef names is an essential component of the
abstraction.  Without it, we end up going to void* and loosing all
type safety.

  That would be my preference.  However, if folks want a shorter name,
  I'll live with that too.  But as it stands, the current name is very
  confusing.

 I would even prefer 'e' over value_type.  It's scoped, the context always
 will be clear, no need to be verbose in that name.  I find the long names
 inelegant, as most of the standard libs conventions.

We need some convention.  If we choose a convention different from
the standard library, then we are essentially saying that we do not
intend to interoperate with the standard library.  I do not think
that is the intent of the community, but I could be wrong about that.

-- 
Lawrence Crowl

Re: Merge C++ conversion into trunk (4/6 - hash table rewrite)

[Lawrence Crowl]

On 9/28/12, Gabriel Dos Reis g...@integrable-solutions.net wrote:
 On Fri, Sep 28, 2012 at 8:18 AM, Michael Matz m...@suse.de wrote:
 It is common practice to have

 struct Q
 {
   typedef int E;
   E getme (int index);
 };

 Easy: I wouldn't make a typedef for Q::E that merely is int.  The reason
 is that it makes knowing what getme really returns harder.

 The point of these nested type is precisely to allow  a *uniform* access
 to associated from within a template (e.g. a container) -- irrespective of
 what those types happen to resolve to, builtin or not.

Perhaps an analogy might be helpful.  If I say Tim's father you know
role of that person without knowing exactly who it is.  The typedef
convention serves the same purpose.  It is important that we use the
same term for father, or we wouldn't be able to communicate.

-- 
Lawrence Crowl

Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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 dirent.h

 #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 char
 {
-  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 hashval_t
+charstar_hash::hash (const T *candidate)
+{
+  return htab_hash_string (candidate);
 }

+inline bool
+charstar_hash::equal (const T *existing, const T *candidate)
+{
+  return strcmp (existing, candidate) == 0;
+}
+
+
 /* A hash table keeping track of class names that were not found
during class lookup.  (There is no need to cache the values
associated with names that were found; they are saved in
IDENTIFIER_CLASS_VALUE.)  */
-static htab_t

Re: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

On 10/2/12, Richard Guenther rguent...@suse.de 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_copy_hasher bb_original;
 +static hash_table bb_copy_hasher 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?

 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

Updated Patch

Add functions symtab_node_def::try_function and symtab_node_def::try_variable.
These function return a pointer to the more specific type (e.g. cgraph_node*)
if and only if the general type (symtab_node aka symtab_node_def*) is an
instance of the specific type.  These functions are essentially checked down
casts.

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 = node-try_function ())
{
  
}

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 = node-try_variable ();
  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 member functions symtab_node_def::is_function and
symtab_node_def::is_variable.  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.


The new code bootstraps .616% faster with a 99% confidence of being faster.


Tested on x86_64.


Okay for trunk?


Index: gcc/ChangeLog

2012-10-02  Lawrence Crowl  cr...@google.com

* cgraph.h (varpool_node): Rename to varpool_node_for_decl.
Adjust callers to match.
(symtab_node_def::try_function): New.
Change most calls to symtab_function_p with calls to
symtab_node_def::try_function.
(symtab_node_def::try_variable): New.
Change most calls to symtab_variable_p with calls to
symtab_node_def::try_variable.
(symtab_function_p): Rename to symtab_node_def::is_function.
Adjust remaining callers to match.
(symtab_variable_p): Rename to symtab_node_def::is_variable.
Adjust remaining callers to match.
* 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.


Index: gcc/lto-symtab.c
===
--- gcc/lto-symtab.c(revision 192010)
+++ gcc/lto-symtab.c(working copy)
@@ -566,11 +566,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 = e-try_function ();
+  if (ce  !DECL_BUILT_IN (e-symbol.decl))
+   lto_cgraph_replace_node (ce, cgraph (prevailing));
+  if (varpool_node *ve = e-try_variable ())
+   lto_varpool_replace_node (ve, varpool (prevailing));
 }

   return;
Index: gcc/cgraphbuild.c
===
--- gcc/cgraphbuild.c   (revision 192010)
+++ 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);
+ struct varpool_node *vnode = varpool_node_for_decl (type);
  ipa_record_reference ((symtab_node)node,
(symtab_node)vnode,
IPA_REF_ADDR, NULL);
@@ -233,7 +233,7 @@ mark_address (gimple stmt, tree addr

Re: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

On 10/2/12, Ian Lance Taylor i...@google.com wrote:
 On Oct 2, 2012 Lawrence Crowl cr...@googlers.com wrote:
  On 10/2/12, Richard Guenther rguent...@suse.de 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

Add myself to MAINTAINERS

[Lawrence Crowl]

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.

[Lawrence Crowl]

On 10/2/12, Richard Guenther rguent...@suse.de 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_copy_hasher bb_original;
 +static hash_table bb_copy_hasher 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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  cr...@google.com

  * 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 dirent.h

  #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

Re: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

Sorry, one more time with the right file contents.

On 10/2/12, Richard Guenther rguent...@suse.de 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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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  cr...@google.com

* 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 dirent.h

 #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 char
 {
-  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

Re: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

On 10/4/12, Richard Guenther rguent...@suse.de wrote:
 On Tue, 2 Oct 2012, Lawrence Crowl wrote:
 On 10/2/12, Richard Guenther rguent...@suse.de 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_copy_hasher bb_original;
  +static hash_table bb_copy_hasher 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  cr...@google.com
 
* 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  cr...@google.com
 
* 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  cr...@google.com
 
* 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  cr...@google.com
 
* 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  cr...@google.com
 
* 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

Re: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

On 10/5/12, Richard Guenther rguent...@suse.de wrote:
 On Thu, 4 Oct 2012, Lawrence Crowl wrote:
 On 10/4/12, Richard Guenther rguent...@suse.de wrote:
  On Tue, 2 Oct 2012, Lawrence Crowl wrote:
  On 10/2/12, Richard Guenther rguent...@suse.de 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_copy_hasher bb_original;
   +static hash_table bb_copy_hasher 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 pointer_hash whatever  x
= hash_table pointer_hash whatever  x ();

I'm guessing there would be some objection to typing that.
The alternative is an init member function.

  hash_table pointer_hash whatever  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.

[Lawrence Crowl]

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 cr...@googlers.com wrote:
 Sorry, one more time with the right file contents.

 On 10/2/12, Richard Guenther rguent...@suse.de 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  cr...@google.com

   * 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  cr...@google.com

   * 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  cr...@google.com

   * 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  cr...@google.com

   * 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  cr...@google.com

   * 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 dirent.h

  #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 char
  {
 -  return strcmp ((const char

Re: Use conditional casting with symtab_node

[Lawrence Crowl]

On 10/5/12, Diego Novillo dnovi...@google.com wrote:
 On Oct 5, 2012 Richard Guenther richard.guent...@gmail.com 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.

[Lawrence Crowl]

On 10/5/12, Jakub Jelinek ja...@redhat.com 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: Convert more non-GTY htab_t to hash_table.

[Lawrence Crowl]

On 10/5/12, Diego Novillo dnovi...@google.com wrote:
 On Oct 3, 2012 Lawrence Crowl cr...@googlers.com 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: Use conditional casting with symtab_node

[Lawrence Crowl]

On 10/5/12, Jan Hubicka hubi...@ucw.cz wrote:
 On Thu, Oct 4, 2012 at 8:16 PM, Diego Novillo dnovi...@google.com wrote:
  On Thu, Oct 4, 2012 at 2:14 PM, Lawrence Crowl cr...@googlers.com 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: Use conditional casting with symtab_node

[Lawrence Crowl]

On 10/10/12, Xinliang David Li davi...@google.com wrote:
 In a different thread, I proposed the following alternative to 'try_xxx':

 templatetypename T T* symbol::cast_to(symbol* p) {
if (p-isT())
   return static_castT*(p);
return 0;
  }

 cast:

 templatetypename T T symbol:as(symbol* p) {
assert(p-isT())
return static_castT(*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 cgraph_node * ())

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

[Lawrence Crowl]

On 10/12/12, Richard Biener richard.guent...@gmail.com wrote:
 On Oct 11, 2012 Xinliang David Li davi...@google.com wrote:
 On Oct 11, 2012 Lawrence Crowl cr...@googlers.com wrote:
 On 10/10/12, Xinliang David Li davi...@google.com wrote:
 In a different thread, I proposed the following alternative to
 'try_xxx':

 templatetypename T T* symbol::cast_to(symbol* p) {
if (p-isT())
   return static_castT*(p);
return 0;
  }

 cast:

 templatetypename T T symbol:as(symbol* p) {
assert(p-isT())
return static_castT(*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 cgraph_node * ())

 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 T *tree::cast_to ()
 {
if (kind_ == kind_traitsT::value)
 return static_castT* (this);

  return 0;
 }

 I also think that instead of

   if (cgraph_node *q = p-cast_to cgraph_node * ())

 we want

   if ((q = cast_to cgraph_node * (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 function * (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 INTEGER_CST (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 tree_common
 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: Add usage documentation for hash_table

[Lawrence Crowl]

, Allocator
 -::traverse_noresize (Argument argument)
 +hash_table Descr, Allocator::traverse_noresize (Argument argument)
  {
T **slot;
T **limit;
 @@ -712,13 +805,11 @@ hash_table Descr, Allocator
  /* Like traverse_noresize, but does resize the table when it is too empty
 to improve effectivity of subsequent calls.  */

 -template typename Descr,
 -   template typename Type class Allocator
 +template typename Descr, template typename Type class Allocator
  template typename Argument,
 int (*Callback) (typename Descr::T **slot, Argument argument)
  void
 -hash_table Descr, Allocator
 -::traverse (Argument argument)
 +hash_table Descr, Allocator::traverse (Argument argument)
  {
size_t size = htab-size;
if (elements () * 8  size  size  32)



-- 
Lawrence Crowl

Re: [PATCH][RFC] Re-organize how we stream trees in LTO

[Lawrence Crowl]

On 10/16/12, Diego Novillo dnovi...@google.com 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: wide int patch #6: Replacement of hwi extraction from int-csts.

[Lawrence Crowl]

On 10/19/12, Richard Biener richard.guent...@gmail.com 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 to fix constant math - 4th patch - the wide-int class.

[Lawrence Crowl]

On 10/23/12, Richard Biener richard.guent...@gmail.com 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: patch to fix constant math - 4th patch - the wide-int class.

[Lawrence Crowl]

On 10/23/12, Kenneth Zadeck zad...@naturalbridge.com 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.

[Lawrence Crowl]

On 10/23/12, Kenneth Zadeck zad...@naturalbridge.com wrote:
 On 10/23/2012 02:38 PM, Lawrence Crowl wrote:
 On 10/23/12, Kenneth Zadeck zad...@naturalbridge.com 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.

 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.

 i have, and it does not work.

Unfortunate.

-- 
Lawrence Crowl

[patch] Hash table changes from cxx-conversion branch - config part

[Lawrence Crowl]

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  cr...@google.com

* 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 bundle_state
+{
+  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_state, state2-dfa_state,
 dfa_state_size) == 0);
 }

+/* Hash table of the bundle states.  The key

Re: [patch] Hash table changes from cxx-conversion branch

[Lawrence Crowl]

On 4/25/13, Richard Biener richard.guent...@gmail.com 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

Re: [patch] Hash table changes from cxx-conversion branch

[Lawrence Crowl]

On 4/25/13, Diego Novillo dnovi...@google.com 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 - config part

[Lawrence Crowl]

I still have not heard from i386 or ia64 folks.  Anyone?

On 4/24/13, Lawrence Crowl cr...@googlers.com 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  cr...@google.com

   * 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 bundle_state
 +{
 +  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

Re: [patch] Hash table changes from cxx-conversion branch - config part

[Lawrence Crowl]

On 5/21/13, Diego Novillo dnovi...@google.com wrote:
 On May 13, 2013 Lawrence Crowl cr...@googlers.com 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: [c++-concepts] code review

[Lawrence Crowl]

On 6/10/13, Diego Novillo dnovi...@google.com wrote:
 On 2013-06-09 20:34 , Gabriel Dos Reis wrote:
  So, my advice is for GCC source code to forget about the cxxx
  headers for the most part.  I can see an instance where cmath
  or cstring would make a difference but given point (1) above,
  no it doesn't.  Just use the traditional xxx.h 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: RFA: PATCH to restore old behavior of debug_vec_tree

[Lawrence Crowl]

On 7/8/13, Jason Merrill ja...@redhat.com 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 vectree,va_gc and changing
 debug_vec_tree to use that.

 OK for trunk?

Looks good to me.

-- 
Lawrence Crowl

Re: [cxx-conversion] RFC - Helper types for building GIMPLE

[Lawrence Crowl]

.  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: [patch] Unified debug dump function names.

[Lawrence Crowl]

On 3/25/13, Richard Biener richard.guent...@gmail.com 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: [patch] Unified debug dump function names.

[Lawrence Crowl]

On 3/25/13, Tom Tromey tro...@redhat.com wrote:
 Lawrence == Lawrence Crowl cr...@googlers.com 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.

[Lawrence Crowl]

On 3/26/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mar 25, 2013 Lawrence Crowl cr...@googlers.com wrote:
  On 3/25/13, Richard Biener richard.guent...@gmail.com 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)
 function_decl 0x76e1b900 foo
type function_type 0x76d28c78
type integer_type 0x76d175e8 int public SI
size integer_cst 0x76d1a0c0 constant 32
unit size integer_cst 0x76d1a0e0 constant 4
align 32 symtab 0 alias set -1 canonical type
 0x76d175e8 precision 32 min integer_cst 0x76d1a060
 -2147483648 max integer_cst 0x76d1a080 2147483647
 ...
 (gdb) call debug_tree (0x76d175e8)
 Cannot resolve function debug_tree to any overloaded instance
 (gdb) call debug_treetabtab
 debug_tree(tree_node*)
 debug_tree_chain(tree_node*)
 debug_tree_chain(tree_node*)::__FUNCTION__
 debug_tree_ssa()
 debug_tree_ssa_stats()
 aha! (ok, I know this one is 'tree')
 (gdb) call debug_tree ((tree_node*)0x76d175e8)
 integer_type 0x76d175e8 int public SI
size integer_cst 0x76d1a0c0 type integer_type 0x76d170a8
 bitsizetype constant 32
unit size integer_cst 0x76d1a0e0 type integer_type
 0x76d17000 sizetype constant 4
align 32 symtab 0 alias set -1 canonical type 0x76d175e8
 precision 32 min integer_cst 0x76d1a060 -2147483648 max
 integer_cst 0x76d1a080 2147483647
pointer_to_this pointer_type 0x76d1f2a0

 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 cutpasted 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_tabtab
 ... endless list of functions and overloads ...

 is probably as useless as

 (gdb) call debugtabtab

 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.

[Lawrence Crowl]

On 3/26/13, Lawrence Crowl cr...@googlers.com wrote:
 On 3/26/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mar 25, 2013 Lawrence Crowl cr...@googlers.com wrote:
  On 3/25/13, Richard Biener richard.guent...@gmail.com 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)
 function_decl 0x76e1b900 foo
type function_type 0x76d28c78
type integer_type 0x76d175e8 int public SI
size integer_cst 0x76d1a0c0 constant 32
unit size integer_cst 0x76d1a0e0 constant 4
align 32 symtab 0 alias set -1 canonical type
 0x76d175e8 precision 32 min integer_cst 0x76d1a060
 -2147483648 max integer_cst 0x76d1a080 2147483647
 ...
 (gdb) call debug_tree (0x76d175e8)
 Cannot resolve function debug_tree to any overloaded instance
 (gdb) call debug_treetabtab
 debug_tree(tree_node*)
 debug_tree_chain(tree_node*)
 debug_tree_chain(tree_node*)::__FUNCTION__
 debug_tree_ssa()
 debug_tree_ssa_stats()
 aha! (ok, I know this one is 'tree')
 (gdb) call debug_tree ((tree_node*)0x76d175e8)
 integer_type 0x76d175e8 int public SI
size integer_cst 0x76d1a0c0 type integer_type 0x76d170a8
 bitsizetype constant 32
unit size integer_cst 0x76d1a0e0 type integer_type
 0x76d17000 sizetype constant 4
align 32 symtab 0 alias set -1 canonical type 0x76d175e8
 precision 32 min integer_cst 0x76d1a060 -2147483648 max
 integer_cst 0x76d1a080 2147483647
pointer_to_this pointer_type 0x76d1f2a0

 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 cutpasted 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_tabtab
 ... endless list of functions and overloads ...

 is probably as useless as

 (gdb) call debugtabtab

 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 'nil' 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

Re: [patch] Unified debug dump function names.

[Lawrence Crowl]

On 3/27/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mar 27, 2013, Lawrence Crowl cr...@googlers.com 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 'nil' 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,

 veccp_token, va_gc, vecdata_reference_p, vecddr_p,
 vecrtx, vectree, va_gc,

 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 cp_token (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, nil\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,
 nil\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] Hash table changes from cxx-conversion branch

[Lawrence Crowl]

On 3/27/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mar 23, 2013 Lawrence Crowl cr...@googlers.com 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.

[Lawrence Crowl]

On 3/28/13, Richard Biener richard.guent...@gmail.com wrote:
 The patch is ok as-is.

Committed.

-- 
Lawrence Crowl

[patch] Remove unused symbols.

[Lawrence Crowl]

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  cr...@google.com

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,
vectree , 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

[patch] Remove unused code from dse.c.

[Lawrence Crowl]

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  cr...@google.com

* 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.  */
-  bitmap_and_compl_into (group-store2_p, disqualified_clear_alias_sets);
-
-  /* We do not need to process the store2_n set because
- alias_sets

Re: [patch] Remove unused code from dse.c.

[Lawrence Crowl]

On 3/29/13, Jeff Law l...@redhat.com 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  cr...@google.com

  * 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

Re: [patch] Remove unused code from dse.c.

[Lawrence Crowl]

On 3/29/13, Jeff Law l...@redhat.com 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

[patch] Remove unused ivtype_map symbols from sese.[hc]

[Lawrence Crowl]

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  cr...@google.com

* 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 ivtype_map symbols from sese.[hc]

[Lawrence Crowl]

On 4/2/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mon, Apr 1, 2013 at 12:19 AM, Lawrence Crowl cr...@googlers.com 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

Re: Comments on the suggestion to use infinite precision math for wide int.

[Lawrence Crowl]

On 4/8/13, Kenneth Zadeck zad...@naturalbridge.com 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_signedT::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: Comments on the suggestion to use infinite precision math for wide int.

[Lawrence Crowl]

On 4/8/13, Richard Biener richard.guent...@gmail.com 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.

[Lawrence Crowl]

On 4/8/13, Robert Dewar de...@adacore.com 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: [patch] Hash table changes from cxx-conversion branch

[Lawrence Crowl]

Ping?

On 3/31/13, Lawrence Crowl cr...@googlers.com wrote:
 On 3/28/13, Richard Biener richard.guent...@gmail.com wrote:
 On Mar 27, 2013 Lawrence Crowl cr...@googlers.com wrote:
  On 3/27/13, Richard Biener richard.guent...@gmail.com wrote:
   On Mar 23, 2013 Lawrence Crowl cr...@googlers.com 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