Re: 0005-Search-all-dominated-blocks-for-expressions-to-hoist.patch

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

On 7/15/10 12:58 AM, Jeff Law wrote:
> /* Collect expressions which might trap. */
> trapping_expr = sbitmap_alloc (expr_hash_table.n_elems);
> sbitmap_zero (trapping_expr);
> 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);
> }
...
> FOR_EACH_EDGE (e, ei, bb->preds)
> if (e->flags & EDGE_ABNORMAL)
> {
> sbitmap_difference (antloc[bb->index], antloc[bb->index], trapping_expr);
> sbitmap_difference (transp[bb->index], transp[bb->index], trapping_expr);
> break;
> }
...
> compute_local_properties ( ... )
> compute_potentially_trapping_expressions ( ... )
>
> FOR_EACH_BB (bb)
> prune_antloc & transp (bb, antloc, transp, trapping_expr);

This will have the effect of disabling hoisting for all potentially 
trapping expressions, even if they don't result in abnormal control 
flow.  E.g., a memory reference will not be eligible for hoisting in 
under these conditions.

[IIUC, it is OK to hoist a potentially trapping expression as long as it 
will trap on every path and provided non-call exceptions are disabled.]

> That avoids having two hunks of code that are supposed to do the same
> thing, but due to implementation differences actually behave differently.

PRE code for handling trapping expressions removes quite a larger set of 
expression from optimization space, and that seems like the right thing 
to do for PRE.  Hoisting, on the other hand can relax condtions on 
trapping expressions by compensating with the VBE requirement.  It seems 
to me that we shouldn't disable hoisting of all potentially trapping 
expressions, but only of those that have an abnormal edge sticking out 
of them.


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

Thanks!

--
Maxim Kuvyrkov
CodeSourcery
maxim@codesourcery.com
(650) 331-3385 x724