On 12/07/2014 14:18, Roman Gareev wrote:
I've attached the patch, which contains generation of Gimple code from
isl_ast_node_user.
I think that it would be better to add motivation for the following
line from the original source:
if (GBB_BB (gbb) == ENTRY_BLOCK_PTR_FOR_FN (cfun))
{
isl_ast_expr_free (user_expr);
return next_e;
}
Do you know anything about this?
No, no idea. To my understanding the entry block should not even appear
within a scop (see build_scops, where we only start detecting scops at
the successor of the entry_block). Maybe we replace this with an assert
to get a good error message in case I have missed something.
-/* IVS_PARAMS maps ISL's scattering and parameter identifiers
+/* TREE_FROM_ISL_ID maps ISL's scattering and parameter identifiers
to corresponding trees. */
-typedef std::map<isl_id *, tree> ivs_params;
+typedef struct ivs_params {
+ std::map<isl_id *, tree> tree_from_isl_id;
+ sese region;
I think this region is actually not needed. At the place where you need
it, you have a pbb available, from which you can obtain a pointer to the
surrounding scop and from this you can obtain this region itself.
+} *ivs_params_p;
case isl_ast_op_lt:
{
// (iterator < ub) => (iterator <= ub - 1)
- isl_val *one = isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond),
1);
+ isl_val *one =
+ isl_val_int_from_si (isl_ast_expr_get_ctx (for_cond), 1);
This is a pure style change which seems unrelated. Also, is the original
line really too long? I may have miscounted, but it seems to fit
exactly.
+/* Inserts in iv_map a tuple (OLD_LOOP->num, NEW_NAME) for the
+ induction variables of the loops around GBB in SESE. */
In polly we just create an iv_map from [0, max-loop-depth-within-scop], so
it contains at most as many elements as the maximal loop depth. Your map
is unnecessarily large, as it contains all loops in the function.
If we can avoid this immediately, e.g. by indexing according to the
loop-depths that would be great. On the other side, if the existing
functions require this, I propose to not touch this area, but to add a
fixme that documents this issue. We can solve it after having removed
the CLooG codegen.
+/* Mark BB with it's relevant PBB via hashing table BB_PBB_MAPPING. */
+
+static void
+mark_bb_with_pbb (poly_bb_p pbb, basic_block bb,
+ bb_pbb_htab_type *bb_pbb_mapping)
+{
+ bool existed;
+ poly_bb_p &e = bb_pbb_mapping->get_or_insert (bb, &existed);
+ if (!existed)
+ e = pbb;
+}
I was just briefly looking the code to remind me what this
bb_pbb_mapping hash table is about, but could not find the reason it is
needed. Do you know why it is needed?
Is it necessary for this patch? Or did you just copy it from the
previous code generation?
+/* Translates an isl_ast_node_user to Gimple. */
+
+static edge
+translate_isl_ast_node_user (__isl_keep isl_ast_node *node,
+ edge next_e, bb_pbb_htab_type *bb_pbb_mapping,
+ ivs_params_p ip)
+{
+ gcc_assert (isl_ast_node_get_type (node) == isl_ast_node_user);
+ int i, nb_loops;
+ basic_block new_bb;
+ isl_ast_expr *user_expr = isl_ast_node_user_get_expr (node);
+ isl_ast_expr *name_expr = isl_ast_expr_get_op_arg (user_expr, 0);
+ gcc_assert (isl_ast_expr_get_type (name_expr) == isl_ast_expr_id);
+ isl_id *name_id = isl_ast_expr_get_id (name_expr);
+ poly_bb_p pbb = (poly_bb_p) isl_id_get_user (name_id);
+ gcc_assert (pbb);
+ gimple_bb_p gbb = PBB_BLACK_BOX (pbb);
+ vec<tree> iv_map;
+ isl_ast_expr_free (name_expr);
+ isl_id_free (name_id);
+
+ if (GBB_BB (gbb) == ENTRY_BLOCK_PTR_FOR_FN (cfun))
+ {
+ isl_ast_expr_free (user_expr);
+ return next_e;
+ }
+
+ nb_loops = number_of_loops (cfun);
+ iv_map.create (nb_loops);
+ for (i = 0; i < nb_loops; i++)
+ iv_map.quick_push (NULL_TREE);
Why do you need to first push NULL_TREEs into this vec, which will be
overwritten right after?
+ build_iv_mapping (iv_map, gbb, user_expr, ip);
+ isl_ast_expr_free (user_expr);
+ next_e = copy_bb_and_scalar_dependences (GBB_BB (gbb), ip->region,
+ next_e, iv_map,
+ &graphite_regenerate_error);
+ iv_map.release ();
+ new_bb = next_e->src;
+ mark_bb_with_pbb (pbb, new_bb, bb_pbb_mapping);
+ mark_virtual_operands_for_renaming (cfun);
+ update_ssa (TODO_update_ssa);
+ return next_e;
+}
+
It is unclear how this patches have been tested. Can you elaborate?
Also, we need to find a way to test this in gcc itself, possibly by
running test cases that already work with both the cloog and the isl ast
generator.
Cheers,
Tobias
