This is the mail archive of the
mailing list for the GCC project.
- From: Jeff Law <law at redhat dot com>
- To: Maxim Kuvyrkov <maxim at codesourcery dot com>
- Cc: Steven Bosscher <stevenb dot gcc at gmail dot com>, gcc-patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 14 Jul 2010 14:58:08 -0600
- Subject: Re: 0005-Search-all-dominated-blocks-for-expressions-to-hoist.patch
- References: <4C18F225.email@example.com> <4C18F491.firstname.lastname@example.org> <AANLkTinT0L8VRpB_Y_sQfoQ4x_tC3CZl5csz6IWJezb2@mail.gmail.com> <4C1FB369.email@example.com> <AANLkTimJKmvB19PiVdPDNh2g6VX9fryFGh4vR_rrm_J3@mail.gmail.com> <4C1FC91D.firstname.lastname@example.org> <4C20AB99.email@example.com> <4C225BA4.firstname.lastname@example.org> <4C237F81.email@example.com> <4C2B8911.firstname.lastname@example.org> <4C2B9B4D.email@example.com> <4C2CC3F4.firstname.lastname@example.org> <4C2E0F06.email@example.com> <4C34AE19.firstname.lastname@example.org> <4C3783E3.email@example.com>
07/09/10 14:17, Maxim Kuvyrkov wrote:
First, don't forget -p when generating your diffs. -p includes the
function's name in the diff output which helps the reviewer understand
where a hunk was changed. Many reviewers prefer unidiffs (-u) as well.
The first of the attached patches replaces transpout with an
additional check in determining if an expression is anticipatable.
Anyway, onward to the patches...
I'm not sure why you didn't just factor out the code from
compute_pre_data and use it for both pre and hoisting.
Pull this fragment from compute_pre_data into its own function and call
it from compute_pre_data and compute_hoist_data:
/* Collect expressions which might trap. */
trapping_expr = sbitmap_alloc (expr_hash_table.n_elems);
for (ui = 0; ui < expr_hash_table.size; ui++)
struct expr *e;
for (e = expr_hash_table.table[ui]; e != NULL; e = e->next_same_hash)
if (may_trap_p (e->expr))
SET_BIT (trapping_expr, e->bitmap_index);
Which gives you a bitmap of potentially trapping expressions.
Then pull this fragment from compute_pre_data into a function:
FOR_EACH_EDGE (e, ei, bb->preds)
if (e->flags & EDGE_ABNORMAL)
sbitmap_difference (antloc[bb->index], antloc[bb->index],
sbitmap_difference (transp[bb->index], transp[bb->index],
compute_pre_data would then look like:
compute_local_properties ( ... )
sbitmap_vector_zero ( ... )
compute_potentially_trapping_expressions ( ...)
/* If the current block is the destination of an abnormal edge, we
kill all trapping expressions because we won't be able to properly
place the instruction on the edge. So make them neither
anticipatable nor transparent. This is fairly conservative. */
prune_antloc & transp (bb, antloc, transp, trapping_expr)
sbitmap_a_or_b (ae_kill[bb->index], transp[bb->index],
sbitmap_not (ae_kill[bb->index], ae_kill[bb->index]);
And compute_code_hoist_data would look like:
compute_local_properties ( ... )
compute_potentially_trapping_expressions ( ... )
prune_antloc & transp (bb, antloc, transp, trapping_expr);
Or something very close to that.
That avoids having two hunks of code that are supposed to do the same
thing, but due to implementation differences actually behave differently.
This looks pretty good. I still see a reference to compute_transpout,
which presumably will go away once we settle on the final form of the
first patch. This patch is OK.
The second patch implements LCA approach to avoid hoisting expression
too far up. As a side effect of implementation, it somewhat
simplifies control flow of hoist_code.
:-) It happens like this sometimes.
I really hope this is the last iteration on the one-line change this
problem initially was :).