On Mon, Oct 26, 2015 at 11:30 AM, Richard Sandiford <richard.sandif...@arm.com> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On Mon, Oct 26, 2015 at 10:41 AM, Richard Sandiford >> <rdsandif...@googlemail.com> wrote: >>> An upcoming patch adds a match.pd rule that folds pow(pow(x,y),z) >>> to pow(x,y*z). This fold can reuse the existing pow gimple statement >>> and simply replace the operands with x and y*z. However, the y*z >>> itself requires a separate gimple statement and the code wasn't >>> prepared to handle that. >>> >>> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi. >>> OK to install? >> >> Hmm, I put the assert there because of the !inplace case but I see the >> is_tree_code case alredy behaves the same. >> >> I think the intent of fold_stmt_inplace (and thus the 'inplace' case) >> was that no additional stmts are inserted (and obviously the >> stmt itself being not replaced, so gsi_stmt () is the same before and >> after). >> >> So I think both the is_gimple_assign && is_tree_code and the >> case you are amending need a >> >> if (inplace && !gimple_seq_empty_p (*seq)) >> return false; > > OK. I was wondering about that, but the comment isn't clear whether > inserting extra statements before the original statement is OK, as long > as the final statement has the same form. > > If this hasn't caused problems for tree codes, do you think that means > that callers don't care about extra statements matter in practice, > that there simply aren't many fold patterns like this, or that most > cases where this kind of fold hits are caught earlier? Just worried > about introducing a pessimisation...
There are not many passes restricting folding to inplace anymore. Maybe it's again time to re-evaluate some of them. > > Should we simply have that check before: > > if (gcond *cond_stmt = dyn_cast <gcond *> (stmt)) > > so that it's common to all cases? Yeah, that makes sense. Thanks, Richard. > Thanks, > Richard >
