On Mon, Jan 12, 2015 at 4:00 PM, Yuri Rumyantsev <ysrum...@gmail.com> wrote: > Hi All, > > Thanks a lot for your comments. > I've re-written reorder_operands as you proposed, but I'd like to know > if we should apply this reordering at -O0?
No, I think we can spare those cycles there. Richard. > I will re-send the patch after testing completion. > Thanks. > Yuri. > > 2015-01-09 13:13 GMT+03:00 Richard Biener <richard.guent...@gmail.com>: >> On Mon, Jan 5, 2015 at 9:26 PM, Jeff Law <l...@redhat.com> wrote: >>> On 12/29/14 06:30, Yuri Rumyantsev wrote: >>>> >>>> Hi All, >>>> >>>> Here is a patch which fixed several performance degradation after >>>> operand canonicalization (r216728). Very simple approach is used - if >>>> operation is commutative and its second operand required more >>>> operations (statements) for computation, swap operands. >>>> Currently this is done under special option which is set-up to true >>>> only for x86 32-bit targets ( we have not seen any performance >>>> improvements on 64-bit). >>>> >>>> Is it OK for trunk? >>>> >>>> 2014-12-26 Yuri Rumyantsev <ysrum...@gmail.com> >>>> >>>> * cfgexpand.c (count_num_stmt): New function. >>>> (reorder_operands): Likewise. >>>> (expand_gimple_basic_block): Insert call of reorder_operands. >>>> * common.opt(flag_reorder_operands): Add new flag. >>>> * config/i386/i386.c (ix86_option_override_internal): Add setup of >>>> flag_reorder_operands for 32-bit target only. >>>> * (doc/invoke.texi: Add new optimization option -freorder-operands. >>>> >>>> gcc/testsuite/ChangeLog >>>> * gcc.target/i386/swap_opnd.c: New test. >>> >>> I'd do this unconditionally -- I don't think there's a compelling reason to >>> add another flag here. >> >> Indeed. >> >>> Could you use estimate_num_insns rather than rolling your own estimate code >>> here? All you have to do is setup the weights structure and call the >>> estimation code. I wouldn't be surprised if ultimately the existing insn >>> estimator is better than the one you're adding. >> >> Just use eni_size_weights. >> >> Your counting is quadratic, that's a no-go. You'd have to keep a lattice >> of counts for SSA names to avoid this. There is swap_ssa_operands (), >> in your swapping code you fail to update SSA operands (maybe non-fatal >> because we are just expanding to RTL, but ...). >> >> bb->loop_father is always non-NULL, but doing this everywhere, not only >> in loops looks fine to me. >> >> You can swap comparison operands on GIMPLE_CONDs for all >> codes by also swapping the EDGE_TRUE_VALUE/EDGE_FALSE_VALUE >> flags on the outgoing BB edges. >> >> There are more cases that can be swapped in regular stmts as well, >> but I suppose we don't need to be "complete" here. >> >> So, in reorder_operands I'd do (pseudo-code) >> >> n = 0; >> for-all-stmts >> gimple_set_uid (stmt, n++); >> lattice = XALLOCVEC (unsigned, n); >> >> i = 0; >> for-all-stmts >> this_stmt_cost = estimate_num_insns (stmt, &eni_size_weights); >> lattice[i] = this_stmt_cost; >> FOR_EACH_SSA_USE_OPERAND () >> if (use-in-this-BB) >> lattice[i] += lattice[gimple_uid (SSA_NAME_DEF_STMT)]; >> i++; >> swap-if-operand-cost says so >> >> Richard. >> >>> Make sure to reference the PR in the ChangeLog. >>> >>> Please update and resubmit. >>> >>> Thanks, >>> Jeff
