On Mon, Jan 5, 2015 at 9:26 PM, Jeff Law <[email protected]> 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 <[email protected]>
>>
>> * 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
