On May 1, 2015 12:27:17 PM GMT+02:00, Eric Botcazou <ebotca...@adacore.com>
wrote:
>> Hmm, special-casing this in the inliner looks odd to me. ISTR the
>inliner
>> already propagates constant parameters to immediate uses, so I guess
>> you run into the casting case you handle specially.
>
>Right on both counts, the original GIMPLE looks like:
>
> right.3 = (system__storage_elements__integer_address) right;
> D.4203 = D.4201 % right.3;
> D.4200 = (system__storage_elements__storage_offset) D.4203;
> return D.4200;
>
>and setup_one_parameter has:
>
>/* If there is no setup required and we are in SSA, take the easy route
> replacing all SSA names representing the function parameter by the
> SSA name passed to function.
>
> We need to construct map for the variable anyway as it might be used
> in different SSA names when parameter is set in function.
>
> Do replacement at -O0 for const arguments replaced by constant.
> This is important for builtin_constant_p and other construct requiring
> constant argument to be visible in inlined function body. */
> if (gimple_in_ssa_p (cfun) && rhs && def && is_gimple_reg (p)
> && (optimize
> || (TREE_READONLY (p)
> && is_gimple_min_invariant (rhs)))
> && (TREE_CODE (rhs) == SSA_NAME
> || is_gimple_min_invariant (rhs))
> && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (def))
> {
> insert_decl_map (id, def, rhs);
> return insert_init_debug_bind (id, bb, var, rhs, NULL);
> }
>
>and here is the GIMPLE after inlining:
>
> _17 = _16;
> _18 = _17;
> right.3_19 = 8;
> _20 = _18 % right.3_19;
> _21 = (system__storage_elements__storage_offset) _20;
>
>so the constant replacement is done for right.3_19 by the above block.
>
>> But then if any constant propagation is ok at -O0 why not perform
>> full-fledged constant propagation (with !DECL_IGNORED_P vars as a
>> propagation barrier) as a regular SSA pass? That is, if you'd had a
>> Inline_Always function like
>>
>> int foo (int i)
>> {
>> return (i + 1) / i;
>> }
>>
>> you'd not get the desired result as the i + 1 stmt wouldn't be folded
>> and thus the (i + 1) / i stmt wouldn't either.
>
>That's OK, only the trivial cases need to be dealt with since it's -O0
>so
>running a fully-fledged constant propagation seems overkill.
>
>> That said - why does RTL optimization not save you here anyway?
>> After all, it is responsible for turning divisions into shifts.
>
>You mean the RTL expander? Because the SSA name isn't replaced with
>the RHS
>of its defining statement in:
>
> /* For EXPAND_INITIALIZER try harder to get something simpler. */
> if (g == NULL
> && modifier == EXPAND_INITIALIZER
> && !SSA_NAME_IS_DEFAULT_DEF (exp)
> && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp)))
> && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
> g = SSA_NAME_DEF_STMT (exp);
>
>since modifier is EXPAND_NORMAL. Do you think it would be OK to be a
>little
>more aggressive here? Something like:
>
> /* If we aren't on the LHS, look into the defining statement. */
> if (g == NULL
> && modifier != EXPAND_WRITE
> && !SSA_NAME_IS_DEFAULT_DEF (exp)
> && (optimize || DECL_IGNORED_P (SSA_NAME_VAR (exp)))
> && stmt_is_replaceable_p (SSA_NAME_DEF_STMT (exp)))
> {
> g = SSA_NAME_DEF_STMT (exp);
> /* For EXPAND_INITIALIZER substitute in all cases, otherwise do
> it only for constants. */
> if (modifier != EXPAND_INITIALIZER
> && (!gimple_assign_copy_p (g)
> || !is_gimple_min_invariant (gimple_assign_rhs1 (g))))
> g = NULL;
> }
>
>That's certainly sufficient here.
Yeah, I think that's a way better place for the hack.
Richard.
