This is the mail archive of the gcc-patches@gcc.gnu.org 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: [PR tree-optimization/52558]: RFC: questions on store data race


On 04/25/12 06:45, Richard Guenther wrote:
On Tue, Apr 24, 2012 at 7:43 PM, Aldy Hernandez<aldyh@redhat.com> wrote:
On 04/13/12 03:46, Richard Guenther wrote:

On Fri, Apr 13, 2012 at 12:11 AM, Aldy Hernandez<aldyh@redhat.com> wrote:

+  /* ?? Perhaps we should cache this somewhere in the BB, or are
+     multiple levels of empty BB's common. ??  */
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    {
+      int res = bb_in_transaction_1 (e->src);
+      if (res != -1)
+       return (bool) res;
+    }
+  return false;

that's weird code - if predecessors are not all in or not in a transaction
you return a random value.  So it seems this is somewhat fragile.

If bb status can be derived from looking at a single stmt why is the
transaction-ness not recorded as a basic-block flag then?  Thus,
why do we have a bit in gimple stmts?

How about we get rid of the GIMPLE bit altogether and keep it in the BB flags like you mention? See patch.


+ bool single_exit_p = single_pred_p (ex->dest);

that's a strange variable name ;)

How about loop_has_only_one_exit? :)



+/* Helper function for execute_sm. On every path that sets REF, set + an appropriate flag indicating the store. */ + +static tree +execute_sm_if_changed_flag_set (struct loop *loop, mem_ref_p ref) +{ ... + FOR_EACH_VEC_ELT (mem_ref_loc_p, locs, i, loc) + { + gimple_stmt_iterator gsi; + gimple stmt; + + gsi = gsi_start_bb (gimple_bb (loc->stmt)); + for (; !gsi_end_p (gsi); gsi_next (&gsi)) + if (gsi_stmt (gsi) == loc->stmt)

does not seem to do it on every path but on every REF setter.  And instead
of the innermost loop just use gsi_for_stmt (loc->stmt).

Updated comment. Fixed.



+ /* Emit the load code into the latch, so that we are sure it will + be processed after all dependencies. */ + latch_edge = loop_latch_edge (loop); + load = gimple_build_assign (mem_ssa, unshare_expr (ref->mem)); lim_data = init_lim_data (load); lim_data->max_loop = loop; lim_data->tgt_loop = loop; + gsi_insert_on_edge (latch_edge, load); + load = gimple_build_assign (tmp_var, mem_ssa); + lim_data = init_lim_data (load); + lim_data->max_loop = loop; + lim_data->tgt_loop = loop; + gsi_insert_on_edge (latch_edge, load);

the 2nd assign is no longer a "load", so I suppose you don't need any lim_data
for it?

Re-did all this. See patch.



+ else if (store == STORE_IF_CHANGED_FLAG) + execute_sm_if_changed (ex, ref->mem, mem_ssa, tmp_var, + store_flag);

and this can pass NULL instead of mem_ssa?

Got rid of mem_ssa altogether, as well as the if-changed approach which proved inferior to the flags based approach.


Overall this looks reasonable.  With also handling trapping things I meant
that we should be able to apply store-motion to

int a[256];
void foo (int *p)
{
   int i;
   for (i= 0;i<256;i++)
     if (flag)
       *p = a[i];
}

but we cannot, because the transform

   lsm = *p;
   for (i = 0; i<  256; ++i)
     if (flag)
       lsm = a[i];
   *p = lsm;

even when the store is properly conditional the load is not (and you do not
change that).  The unconditional load may trap here.  What I meant to say
is that we do not need the load at all unless there is also a load involved
inside the loop - if we use the flag-based approach.  If there is also a load
inside the loop we'd need to perform the load there (or not handle this
case)

Ahh! Yes, this sounds very profitable. Would you mind me doing this in a separate patch? I am already handling loads in this incarnation, so this should be straightforward.


Speak of loads, I am keeping the information as an additional bitmap in `memory_accesses', as ->refs_in_loop was set for stores as well, so I couldn't depend on it. Let me know if you have another idea.

Mildly tested. Wadaya think?

Thanks again.
Aldy

Attachment: curr
Description: Text document


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