On Fri, Mar 9, 2012 at 10:48 PM, Aldy Hernandez <al...@redhat.com> wrote: > >> Note that partial PRE (enabled at -O3) can insert expressions into paths >> that did _not_ execute the expression. For regular PRE you are right. >> >> Richard. > > > I've thought about this some more, and Torvald's comment makes a lot of > sense. PRE can make things completely redundant, and a later pass may move > things before its publication. I believe it's wise to disable PRE inside > transactions as originally discussed. The attached patch does so. > > Below is an example (from my patch) with an explanation of what may go > wrong. > > Torvald is this what you were thinking of? > > Richards, is this OK for the 4.7 branch and trunk (pending tests)?
+ if (flag_tm + && gimple_in_transaction (stmt) + && gimple_assign_single_p (stmt)) + { + tree rhs = gimple_assign_rhs1 (stmt); + if (DECL_P (rhs) && is_global_var (rhs)) + continue; this does not cover a read like 'a.b', nor a '*p' read. I think in some other thread I sketched some ref_may_refer_to_global function, maybe you remember. Richard. > Thanks. > Aldy > > + /* Non local loads in a transaction cannot be hoisted out, > + because they may make partially redundant expressions > + totally redundant, which a later pass may move before its > + publication by another thread. > + > + For example: > + > + __transaction_atomic { > + if (flag) > + y = x + 4; > + else > + // stuff > + z = x + 4; > + } > + > + PRE can rewrite this into: > + > + __transaction_atomic { > + if (flag) { > + tmp = x + 4; > + y = tmp; > + } else { > + // stuff > + tmp = x + 4; > + } > + z = tmp; > + } > + > + A later pass can move the now totally redundant [x + 4] > + before its publication predicated by "flag": > + > + __transaction_atomic { > + tmp = x + 4; > + if (flag) { > + } else { > + // stuff > + } > + z = tmp; > + */