Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On Mon, Dec 3, 2012 at 9:14 PM, Lawrence Crowl cr...@googlers.com wrote: On 12/3/12, Diego Novillo dnovi...@google.com wrote: On 2012-12-01 20:44 , Lawrence Crowl wrote: Index: gcc/gimple-fold.c === --- gcc/gimple-fold.c(revision 193902) +++ gcc/gimple-fold.c(working copy) @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. #include tree-ssa-propagate.h #include target.h #include gimple-fold.h +#include gimplify-ctx.h /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. Index: gcc/tree-mudflap.c === --- gcc/tree-mudflap.c (revision 193902) +++ gcc/tree-mudflap.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. #include ggc.h #include cgraph.h #include gimple.h +#include gimplify-ctx.h extern void add_bb_to_loop (basic_block, struct loop *); Index: gcc/tree-inline.c === --- gcc/tree-inline.c(revision 193902) +++ gcc/tree-inline.c(working copy) @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. #include value-prof.h #include tree-pass.h #include target.h +#include gimplify-ctx.h I don't follow. It seems that factoring into gimplify-ctx.h does not actually buy much. The files using it are just including *another* file. Whereas previously, they were getting that content from gimple.h. Unless we can stop including gimple.h from these files, I don't see a lot of gain in this factoring. Am I missing something? There at least 70 files that include gimple.h, and only 5 that need gimple-ctx.h. By splitting it out, at least 65 files will not need to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, the gimplify_hasher template struct, and may not need to include hash-table.h. It's all about avoiding superfluous compilation in other files. But it's backward - gimple.h is the core file as it provides GIMPLE. The gimplifier and its context certainly requires to know about GIMPLE. Btw, if we still want to split it I'd rather have gimplify.h (and also put all exports from gimplify.c there, not only gimplify_ctx). Still I don't think it would buy us much. Richard. -- Lawrence Crowl
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On Tue, Dec 4, 2012 at 4:23 AM, Richard Biener richard.guent...@gmail.com wrote: On Mon, Dec 3, 2012 at 9:14 PM, Lawrence Crowl cr...@googlers.com wrote: On 12/3/12, Diego Novillo dnovi...@google.com wrote: On 2012-12-01 20:44 , Lawrence Crowl wrote: Index: gcc/gimple-fold.c === --- gcc/gimple-fold.c(revision 193902) +++ gcc/gimple-fold.c(working copy) @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. #include tree-ssa-propagate.h #include target.h #include gimple-fold.h +#include gimplify-ctx.h /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. Index: gcc/tree-mudflap.c === --- gcc/tree-mudflap.c (revision 193902) +++ gcc/tree-mudflap.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. #include ggc.h #include cgraph.h #include gimple.h +#include gimplify-ctx.h extern void add_bb_to_loop (basic_block, struct loop *); Index: gcc/tree-inline.c === --- gcc/tree-inline.c(revision 193902) +++ gcc/tree-inline.c(working copy) @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. #include value-prof.h #include tree-pass.h #include target.h +#include gimplify-ctx.h I don't follow. It seems that factoring into gimplify-ctx.h does not actually buy much. The files using it are just including *another* file. Whereas previously, they were getting that content from gimple.h. Unless we can stop including gimple.h from these files, I don't see a lot of gain in this factoring. Am I missing something? There at least 70 files that include gimple.h, and only 5 that need gimple-ctx.h. By splitting it out, at least 65 files will not need to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, the gimplify_hasher template struct, and may not need to include hash-table.h. It's all about avoiding superfluous compilation in other files. But it's backward - gimple.h is the core file as it provides GIMPLE. The gimplifier and its context certainly requires to know about GIMPLE. Btw, if we still want to split it I'd rather have gimplify.h (and also put all exports from gimplify.c there, not only gimplify_ctx). Still I don't think it would buy us much. We talked offline about this with Lawrence. I was not too convinced about adding this new header file, it seemed forced but I was struggling with a better alternative. Perhaps gimplify.h is a better way, but I'm not sure either. We decided that since it's in the branch, it may not matter much for now. However, another thing occurred to me in the meantime: merges. The new file will probably cause merge problems. If we are not completely convinced that it's a good change, we are making our life harder for no gain. Lawrence, you're going to hate me, but would you mind just leaving the ctx structure in gimple.h for now? Sorry! I think it's going to be easier to just leave it there for now until we come up with a good split for the constructs in that file. Diego.
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On 12/4/12, Diego Novillo dnovi...@google.com wrote: On Tue, Dec 4, 2012 at 4:23 AM, Richard Biener richard.guent...@gmail.com wrote: On Mon, Dec 3, 2012 at 9:14 PM, Lawrence Crowl cr...@googlers.com wrote: On 12/3/12, Diego Novillo dnovi...@google.com wrote: On 2012-12-01 20:44 , Lawrence Crowl wrote: Index: gcc/gimple-fold.c === --- gcc/gimple-fold.c(revision 193902) +++ gcc/gimple-fold.c(working copy) @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. #include tree-ssa-propagate.h #include target.h #include gimple-fold.h +#include gimplify-ctx.h /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. Index: gcc/tree-mudflap.c === --- gcc/tree-mudflap.c (revision 193902) +++ gcc/tree-mudflap.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. #include ggc.h #include cgraph.h #include gimple.h +#include gimplify-ctx.h extern void add_bb_to_loop (basic_block, struct loop *); Index: gcc/tree-inline.c === --- gcc/tree-inline.c(revision 193902) +++ gcc/tree-inline.c(working copy) @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. #include value-prof.h #include tree-pass.h #include target.h +#include gimplify-ctx.h I don't follow. It seems that factoring into gimplify-ctx.h does not actually buy much. The files using it are just including *another* file. Whereas previously, they were getting that content from gimple.h. Unless we can stop including gimple.h from these files, I don't see a lot of gain in this factoring. Am I missing something? There at least 70 files that include gimple.h, and only 5 that need gimple-ctx.h. By splitting it out, at least 65 files will not need to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, the gimplify_hasher template struct, and may not need to include hash-table.h. It's all about avoiding superfluous compilation in other files. But it's backward - gimple.h is the core file as it provides GIMPLE. The gimplifier and its context certainly requires to know about GIMPLE. Btw, if we still want to split it I'd rather have gimplify.h (and also put all exports from gimplify.c there, not only gimplify_ctx). Still I don't think it would buy us much. We talked offline about this with Lawrence. I was not too convinced about adding this new header file, it seemed forced but I was struggling with a better alternative. Perhaps gimplify.h is a better way, but I'm not sure either. We decided that since it's in the branch, it may not matter much for now. However, another thing occurred to me in the meantime: merges. The new file will probably cause merge problems. If we are not completely convinced that it's a good change, we are making our life harder for no gain. Lawrence, you're going to hate me, but would you mind just leaving the ctx structure in gimple.h for now? Sorry! I think it's going to be easier to just leave it there for now until we come up with a good split for the constructs in that file. Okay, I'll do that. -- Lawrence Crowl
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On 2012-12-01 20:44 , Lawrence Crowl wrote: Index: gcc/gimple-fold.c === --- gcc/gimple-fold.c (revision 193902) +++ gcc/gimple-fold.c (working copy) @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. #include tree-ssa-propagate.h #include target.h #include gimple-fold.h +#include gimplify-ctx.h /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. Index: gcc/tree-mudflap.c === --- gcc/tree-mudflap.c (revision 193902) +++ gcc/tree-mudflap.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. #include ggc.h #include cgraph.h #include gimple.h +#include gimplify-ctx.h extern void add_bb_to_loop (basic_block, struct loop *); Index: gcc/tree-inline.c === --- gcc/tree-inline.c (revision 193902) +++ gcc/tree-inline.c (working copy) @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. #include value-prof.h #include tree-pass.h #include target.h +#include gimplify-ctx.h I don't follow. It seems that factoring into gimplify-ctx.h does not actually buy much. The files using it are just including *another* file. Whereas previously, they were getting that content from gimple.h. Unless we can stop including gimple.h from these files, I don't see a lot of gain in this factoring. Am I missing something? Diego.
Re: [cxx-conversion] gimplify_ctx::temp_htab hash table
On 12/3/12, Diego Novillo dnovi...@google.com wrote: On 2012-12-01 20:44 , Lawrence Crowl wrote: Index: gcc/gimple-fold.c === --- gcc/gimple-fold.c(revision 193902) +++ gcc/gimple-fold.c(working copy) @@ -30,6 +30,7 @@ along with GCC; see the file COPYING3. #include tree-ssa-propagate.h #include target.h #include gimple-fold.h +#include gimplify-ctx.h /* Return true when DECL can be referenced from current unit. FROM_DECL (if non-null) specify constructor of variable DECL was taken from. Index: gcc/tree-mudflap.c === --- gcc/tree-mudflap.c (revision 193902) +++ gcc/tree-mudflap.c (working copy) @@ -43,6 +43,7 @@ along with GCC; see the file COPYING3. #include ggc.h #include cgraph.h #include gimple.h +#include gimplify-ctx.h extern void add_bb_to_loop (basic_block, struct loop *); Index: gcc/tree-inline.c === --- gcc/tree-inline.c(revision 193902) +++ gcc/tree-inline.c(working copy) @@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. #include value-prof.h #include tree-pass.h #include target.h +#include gimplify-ctx.h I don't follow. It seems that factoring into gimplify-ctx.h does not actually buy much. The files using it are just including *another* file. Whereas previously, they were getting that content from gimple.h. Unless we can stop including gimple.h from these files, I don't see a lot of gain in this factoring. Am I missing something? There at least 70 files that include gimple.h, and only 5 that need gimple-ctx.h. By splitting it out, at least 65 files will not need to parse the gimplify_ctx struct, the gimple_temp_hash_elt struct, the gimplify_hasher template struct, and may not need to include hash-table.h. It's all about avoiding superfluous compilation in other files. -- Lawrence Crowl
[cxx-conversion] gimplify_ctx::temp_htab hash table
Change gimplify.c gimplify_ctx::temp_htab hash table from htab_t to
hash_table.
Move struct gimple_temp_hash_elt and struct gimplify_ctx to a new
gimplify-ctx.h, because they are used few places.
Tested on x86-64.
Okay for branch?
Index: gcc/ChangeLog
2012-11-30 Lawrence Crowl 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
