On Fri, Jun 25, 2010 at 8:34 AM, Eric Botcazou <[email protected]> wrote:
>> Minus whitespace changes it seems to be
>>
>> ! if (lhs_free && (is_gimple_reg (rhs) ||
>> is_gimple_min_invariant (rhs)))
>> rhs_free = true;
>>
>> vs.
>>
>> ! if (lhs_free
>> ! && (is_gimple_reg (rhs)
>> ! || !is_gimple_reg_type (TREE_TYPE (rhs))
>> ! || is_gimple_min_invariant (rhs)))
>> rhs_free = true;
>>
>> so the stmt is likely being eliminated if either the LHS or the RHS is
>> based on a parameter and the other side is a register or an invariant. You
>> change that to also discount aggregate stores/loads to/from parameters to
>> be free.
>
> There is also the counterpart for the RHS:
>
> ! if (rhs_free && is_gimple_reg (lhs))
> lhs_free = true;
> vs
>
> ! if (rhs_free
> ! && (is_gimple_reg (lhs)
> ! || !is_gimple_reg_type (TREE_TYPE (lhs))))
> lhs_free = true;
Sure, but it's requivalent.
>> Which you could have simplified to just say
>>
>> if (lhs_free || rhs_free)
>> return true;
>>
>> and drop the code you are changing.
>
> I don't think so, compare your version and mine for scalar stores/loads
> from/to parameters or return values.
I do think so. Quoting the complete patched code:
if (TREE_CODE (inner_rhs) == PARM_DECL
|| (TREE_CODE (inner_rhs) == SSA_NAME
&& SSA_NAME_IS_DEFAULT_DEF (inner_rhs)
&& TREE_CODE (SSA_NAME_VAR (inner_rhs)) == PARM_DECL))
rhs_free = true;
! if (rhs_free
! && (is_gimple_reg (lhs)
! || !is_gimple_reg_type (TREE_TYPE (lhs))))
lhs_free = true;
if (((TREE_CODE (inner_lhs) == PARM_DECL
|| (TREE_CODE (inner_lhs) == SSA_NAME
&& SSA_NAME_IS_DEFAULT_DEF (inner_lhs)
&& TREE_CODE (SSA_NAME_VAR (inner_lhs)) == PARM_DECL))
&& inner_lhs != lhs)
|| TREE_CODE (inner_lhs) == RESULT_DECL
|| (TREE_CODE (inner_lhs) == SSA_NAME
&& TREE_CODE (SSA_NAME_VAR (inner_lhs)) == RESULT_DECL))
lhs_free = true;
! if (lhs_free
! && (is_gimple_reg (rhs)
! || !is_gimple_reg_type (TREE_TYPE (rhs))
! || is_gimple_min_invariant (rhs)))
rhs_free = true;
if (lhs_free && rhs_free)
return true;
now, with is_gimple_reg () || !is_gimple_reg_type () ||
is_gimple_min_invariant () you allow registers, constants or
any aggregates but _not_ register typed variables that have
their address taken.
Which in the following example makes i = *p not likely eliminated
but makes j = *q likely eliminated.
void foo (int *p, struct X *q)
{
int i;
struct X j;
i = *p;
j = *q;
bar (&i, &q);
}
That doesn't make sense.
What makes sense is that all scalar (thus gimple_reg_typed)
loads/stores to/from parameters or the result are free. Which
isn't what the current code does but is also not what you
are changing it to.
Thus in the above example i = *p should be likely eliminated
but not j = *q (maybe we can make aggregate loads/stores
from/to non-address-taken vars as free, too).
Richard.
> --
> Eric Botcazou
>
