On Wed, Jan 14, 2015 at 11:45 AM, Jakub Jelinek <ja...@redhat.com>
wrote:
> On Wed, Jan 14, 2015 at 01:32:13PM +0300, Yuri Rumyantsev wrote:
>> Sorry, I resend correct patch.
>
>> +reorder_operands (basic_block bb)
>> +{
>> + unsigned int *lattice; /* Hold cost of each statement. */
>> + unsigned int i = 0, n = 0;
>> + gimple_stmt_iterator gsi;
>> + gimple_seq stmts;
>> + gimple stmt;
>> + bool swap;
>> + tree op0, op1;
>> + ssa_op_iter iter;
>> + use_operand_p use_p;
>> + enum tree_code code;
>> + gimple def0, def1;
>> +
>> + if (!optimize)
>> + return;
>
> Wouldn't it be better to move the !optimize guard to the caller?
>
>> + /* Compute cost of each statement using estimate_num_insns. */
>> + stmts = bb_seq (bb);
>> + for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
>> + {
>> + stmt = gsi_stmt (gsi);
>> + gimple_set_uid (stmt, n++);
>> + }
>> + lattice = XALLOCAVEC (unsigned, n);
>
> I'd be afraid that for very large functions you'd ICE here, stack is far
> more limited than heap on many hosts. Either give up if n is say .5 million
> or above, or allocate from heap in that case?
>
>> + for (gsi = gsi_start (stmts); !gsi_end_p (gsi); gsi_next (&gsi))
>> + {
>> + unsigned cost;
>> + stmt = gsi_stmt (gsi);
>> + cost = estimate_num_insns (stmt, &eni_size_weights);
>> + lattice[i] = cost;
>> + FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_USE | SSA_OP_VUSE)
>
> Why the SSA_OP_VUSE?
Shouldn't be needed.
>> + if (gimple_code (stmt) != GIMPLE_ASSIGN
>
> !is_gimple_assign (stmt)
> instead
>
>> + if (op0 ==NULL_TREE || op1 == NULL_TREE
>
> Missing space after ==.
>
>> + /* Swap operands if the second one is more expensive. */
>> + def0 = get_gimple_for_ssa_name (op0);
>> + if (!def0)
>> + continue;
>> + def1 = get_gimple_for_ssa_name (op1);
>> + if (!def1)
>> + continue;
>> + swap = false;
>
> You don't check here if def0/def1 are from the same bb, is that guaranteed?
I think so - we only TER inside BBs.
>> + if (swap)
>> + {
>> + if (dump_file && (dump_flags & TDF_DETAILS))
>> + {
>> + fprintf (dump_file, "Swap operands in stmt:\n");
>> + print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM);
>> + }
>> + gimple_set_op (stmt, 1, op1);
>> + gimple_set_op (stmt, 2, op0);
>
> update_stmt (stmt); ?
Or rather swap_ssa_operands (stmt, gimple_assign_rhs1_ptr (stmt),
gimple_assign_rhs2_ptr (stmt))
>> Index: testsuite/gcc.dg/torture/pr64434.c
>> ===================================================================
>> --- testsuite/gcc.dg/torture/pr64434.c (revision 0)
>> +++ testsuite/gcc.dg/torture/pr64434.c (working copy)
>>
>> Property changes on: testsuite/gcc.dg/torture/pr64434.c
>> ___________________________________________________________________
>> Added: svn:executable
>
> Please don't make testcases executable.
>
>> ## -0,0 +1 ##
>> +*
>> \ No newline at end of property
>
> Please avoid these, terminate with newline.
>
> Jakub
