This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

07/09/10 14:17, Maxim Kuvyrkov wrote:

The first of the attached patches replaces transpout with an additional check in determining if an expression is anticipatable.
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.

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);
  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);

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], trapping_expr);
sbitmap_difference (transp[bb->index], transp[bb->index], trapping_expr);

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], comp[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 ( ... )

  FOR_EACH_BB (bb)
    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.

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.

I really hope this is the last iteration on the one-line change this problem initially was :).
:-) It happens like this sometimes.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]