Richard Guenther <richard.guent...@gmail.com> writes:
>> +/* Expand a call to internal function FN. ARGS is an array of the
>> + function's arguments and LHS is where the result should be stored. */
>> +
>> +void
>> +expand_internal_call (enum internal_fn fn, tree lhs, tree *args)
>> +{
>> + internal_fn_expanders[(int) fn] (lhs, args);
>
> Can you use an interface that simply takes a gimple statement?
> We want to eventually transistion to expanding directly from them
> without re-creating a GENERIC call-expr for 4.7.
OK.
>> /* Return the function type of the function called by GS. */
>>
>> static inline tree
>> gimple_call_fntype (const_gimple gs)
>> {
>> GIMPLE_CHECK (gs, GIMPLE_CALL);
>> - return gs->gimple_call.fntype;
>> + gcc_gimple_checking_assert (!gimple_call_internal_p (gs));
>
> I'm not sure this is the best fallback but it's certainly safe ;)
> I think we should return NULL for internal fns, that is sure to trap
> also for non-checking-enabled builds.
trap == segfault? We'd do that either way on most machines.
But yeah, if you think we should be able to call this for internal
functions, I'll change it.
>> + return gs->gimple_call.u.fntype;
>> }
>>
>> /* Set the type of the function called by GS to FNTYPE. */
>> @@ -2019,7 +2049,8 @@ gimple_call_fntype (const_gimple gs)
>> gimple_call_set_fntype (gimple gs, tree fntype)
>> {
>> GIMPLE_CHECK (gs, GIMPLE_CALL);
>> - gs->gimple_call.fntype = fntype;
>> + gcc_gimple_checking_assert (!gimple_call_internal_p (gs));
>> + gs->gimple_call.u.fntype = fntype;
>> }
>>
>>
>> @@ -2029,8 +2060,12 @@ gimple_call_set_fntype (gimple gs, tree
>> static inline tree
>> gimple_call_fn (const_gimple gs)
>> {
>> + tree op;
>> +
>> GIMPLE_CHECK (gs, GIMPLE_CALL);
>> - return gimple_op (gs, 1);
>> + op = gimple_op (gs, 1);
>> + gcc_gimple_checking_assert (op != NULL_TREE);
>
> Hmmm... probably good for your time of development but please remove
> this. Rather add a hunk to verify_gimple_call that checks that internal
> fns have a NULL fn.
Well, TBH, I had it there because I thought it was a better interface.
I don't think it ever triggered during testing. But again, if you think
it should be OK to call this for internal functions, I'll change it.
>> +/* Set internal function FN to be the function called by call statement GS.
>> */
>> +
>> +static inline void
>> +gimple_call_set_internal_fn (gimple gs, enum internal_fn fn)
>> +{
>> + GIMPLE_CHECK (gs, GIMPLE_CALL);
>> + gs->gsbase.subcode |= GF_CALL_INTERNAL;
>> + gimple_set_op (gs, 1, NULL_TREE);
>
> can we instead assert this? We shouldn't ever change a calls state
> from non-internal to internal (or vice-versa).
OK.
>> + gs->gimple_call.u.internal_fn = fn;
>> +}
>> +
>> +
>> /* Set FN to be the function called by call statement GS. */
>>
>> static inline void
>> gimple_call_set_fn (gimple gs, tree fn)
>> {
>> GIMPLE_CHECK (gs, GIMPLE_CALL);
>> + gs->gsbase.subcode &= ~GF_CALL_INTERNAL;
>
> Likewise. Or just let the stmt verifier catch it.
Which is better? I gather from the above that it's better not to
assert in these accessors.
>> +/* Return true if calls C1 and C2 are known to go to the same function. */
>> +
>> +bool
>> +gimple_call_same_target_p (const_gimple c1, const_gimple c2)
>> +{
>> + if (gimple_call_internal_p (c1))
>> + return (gimple_call_internal_p (c2)
>> + && gimple_call_internal_fn (c1) == gimple_call_internal_fn (c2));
>> + else
>> + return (!gimple_call_internal_p (c2)
>> + && operand_equal_p (gimple_call_fn (c1), gimple_call_fn (c2),
>> 0));
>
> After a patch I am about to commit in a sec. the following for comparing
> gimple_call_fn would be better:
>
> gimple_call_fn (c1) == gimple_call_fn (c2)
> || (gimple_call_fndecl (c1)
> && gimple_call_fndecl (c1) == gimple_call_fndecl (c2))
>
> because we have several forms of specifying a function-decl.
OK.
>> - new_stmt = gimple_build_call_vec (fn, vargs);
>> + if (gimple_call_internal_p (stmt))
>> + new_stmt = gimple_build_call_internal_vec (gimple_call_internal_fn
>> (stmt),
>> + vargs);
>> + else
>> + new_stmt = gimple_build_call_vec (gimple_call_fn (stmt), vargs);
>
> This code looks like it could be simplified by using gimple_copy, a
> loop to set arguments and gimple_set_num_ops. That of course looks
> unrelated to your change, but it makes apparent that sth is wrong with
> this function ;)
Wouldn't that allocate unnecessary ops (as many as the original statement
had, rather than the number that have been kept)? But like you say,
it seems unrelated to this patch, so I'd like to leave it be if that's OK.
>> Index: gcc/expr.c
>> ===================================================================
>> --- gcc/expr.c 2011-04-18 10:18:49.000000000 +0100
>> +++ gcc/expr.c 2011-04-18 10:18:54.000000000 +0100
>> @@ -8521,10 +8521,15 @@ expand_expr_real_1 (tree exp, rtx target
>> enum machine_mode pmode;
>>
>> /* Get the signedness to be used for this variable. Ensure we get
>> - the same mode we got when the variable was declared. */
>> + the same mode we got when the variable was declared.
>> +
>> + Note that calls to internal functions do not result in a
>> + call instruction, so promote_function_mode is not meaningful
>> + in that case. */
>> if (code == SSA_NAME
>> && (g = SSA_NAME_DEF_STMT (ssa_name))
>> - && gimple_code (g) == GIMPLE_CALL)
>> + && gimple_code (g) == GIMPLE_CALL
>> + && !gimple_call_internal_p (g))
>
> I'm not sure we'll never need to do promotions for internal functions
> but at least for now this looks ok, and we can think of how to deal
> with this when it's necessary. Of course it looks fishy that you
> even arrive here ...?
Yeah, I don't think we did arrive here. I was trying to fix all uses of
gimple_call_fn and gimple_call_fntype by inspection. I can assert instead
if you think it really shouldn't ever happen.
>> Index: gcc/gimple-low.c
>> ===================================================================
>> --- gcc/gimple-low.c 2011-04-18 10:18:49.000000000 +0100
>> +++ gcc/gimple-low.c 2011-04-18 10:18:54.000000000 +0100
>> @@ -224,6 +224,8 @@ gimple_check_call_args (gimple stmt, tre
>> /* Get argument types for verification. */
>> if (fndecl)
>> parms = TYPE_ARG_TYPES (TREE_TYPE (fndecl));
>> + else if (gimple_call_internal_p (stmt))
>> + parms = NULL_TREE;
>
> Instead do a
>
> /* Calls to internal functions always match their signature. */
> if (gimple_call_internal_p (stmt))
> return true;
>
> early.
OK.
>> verify_gimple_call (gimple stmt)
>> {
>> - tree fn = gimple_call_fn (stmt);
>> + tree fn;
>> tree fntype, fndecl;
>> unsigned i;
>>
>> - if (!is_gimple_call_addr (fn))
>> + if (!gimple_call_internal_p (stmt))
>
> Please do not re-indent most of the function but instead do an upfront
> conditionl verification for internal calls:
>
> if (gimple_call_internal_p (stmt))
> {
> if (gimple_op (stmt, 1) != NULL_TREE)
> {
> error ("non-NULL function in gimple internal call");
> return true;
> }
> return false;
> }
But what about stuff like:
if (gimple_call_lhs (stmt)
&& (!is_gimple_lvalue (gimple_call_lhs (stmt))
|| verify_types_in_gimple_reference (gimple_call_lhs (stmt), true)))
{
error ("invalid LHS in gimple call");
return true;
}
I left quite a few bits of the function out of the reindentation because
they seemed to apply (or be neutral) for internal functions.
>
>> {
>> - error ("invalid function in gimple call");
>> - debug_generic_stmt (fn);
>> - return true;
>> - }
>> -
>> - if (!POINTER_TYPE_P (TREE_TYPE (fn))
>> - || (TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != FUNCTION_TYPE
>> - && TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != METHOD_TYPE))
>> - {
>> - error ("non-function in gimple call");
>> - return true;
>> + fn = gimple_call_fn (stmt);
>> + if (!is_gimple_call_addr (fn))
>> + {
>> + error ("invalid function in gimple call");
>> + debug_generic_stmt (fn);
>> + return true;
>> + }
>> + if (!POINTER_TYPE_P (TREE_TYPE (fn))
>> + || (TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != FUNCTION_TYPE
>> + && TREE_CODE (TREE_TYPE (TREE_TYPE (fn))) != METHOD_TYPE))
>> + {
>> + error ("non-function in gimple call");
>> + return true;
>> + }
>> + fntype = gimple_call_fntype (stmt);
>> + if (gimple_call_lhs (stmt)
>> + && !useless_type_conversion_p (TREE_TYPE (gimple_call_lhs (stmt)),
>> + TREE_TYPE (fntype))
>> + /* ??? At least C++ misses conversions at assignments from
>> + void * call results.
>> + ??? Java is completely off. Especially with functions
>> + returning java.lang.Object.
>> + For now simply allow arbitrary pointer type conversions. */
>> + && !(POINTER_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))
>> + && POINTER_TYPE_P (TREE_TYPE (fntype))))
>> + {
>> + error ("invalid conversion in gimple call");
>> + debug_generic_stmt (TREE_TYPE (gimple_call_lhs (stmt)));
>> + debug_generic_stmt (TREE_TYPE (fntype));
>> + return true;
>> + }
>> }
>>
>> fndecl = gimple_call_fndecl (stmt);
>> @@ -3086,24 +3106,6 @@ verify_gimple_call (gimple stmt)
>> return true;
>> }
>>
>> - fntype = gimple_call_fntype (stmt);
>> - if (gimple_call_lhs (stmt)
>> - && !useless_type_conversion_p (TREE_TYPE (gimple_call_lhs (stmt)),
>> - TREE_TYPE (fntype))
>> - /* ??? At least C++ misses conversions at assignments from
>> - void * call results.
>> - ??? Java is completely off. Especially with functions
>> - returning java.lang.Object.
>> - For now simply allow arbitrary pointer type conversions. */
>> - && !(POINTER_TYPE_P (TREE_TYPE (gimple_call_lhs (stmt)))
>> - && POINTER_TYPE_P (TREE_TYPE (fntype))))
>> - {
>> - error ("invalid conversion in gimple call");
>> - debug_generic_stmt (TREE_TYPE (gimple_call_lhs (stmt)));
>> - debug_generic_stmt (TREE_TYPE (fntype));
>> - return true;
>> - }
>> -
>> if (gimple_call_chain (stmt)
>> && !is_gimple_val (gimple_call_chain (stmt)))
>> {
>> @@ -3121,7 +3123,7 @@ verify_gimple_call (gimple stmt)
>> error ("static chain in indirect gimple call");
>> return true;
>> }
>> - fn = TREE_OPERAND (fn, 0);
>> + fn = TREE_OPERAND (gimple_call_fn (stmt), 0);
>>
>> if (!DECL_STATIC_CHAIN (fn))
>> {
>> @@ -7436,6 +7438,8 @@ do_warn_unused_result (gimple_seq seq)
>> case GIMPLE_CALL:
>> if (gimple_call_lhs (g))
>> break;
>> + if (gimple_call_internal_p (g))
>> + break;
>>
>> /* This is a naked call, as opposed to a GIMPLE_CALL with an
>> LHS. All calls whose value is ignored should be
>> Index: gcc/tree-eh.c
>> ===================================================================
>> --- gcc/tree-eh.c 2011-04-18 10:18:49.000000000 +0100
>> +++ gcc/tree-eh.c 2011-04-18 10:18:54.000000000 +0100
>> @@ -2743,7 +2743,7 @@ same_handler_p (gimple_seq oneh, gimple_
>> || gimple_call_lhs (twos)
>> || gimple_call_chain (ones)
>> || gimple_call_chain (twos)
>> - || !operand_equal_p (gimple_call_fn (ones), gimple_call_fn (twos), 0)
>> + || !gimple_call_same_target_p (ones, twos)
>
> Why do we even end up here? internal function calls should never throw,
> maybe that is what you need to adjust?
We didn't end up here for internal functions. It was just a case of
using the new API rather than continuing to do a lower-level check.
>> @@ -565,8 +571,14 @@ print_expr_hash_elt (FILE * stream, cons
>> {
>> size_t i;
>> size_t nargs = element->expr.ops.call.nargs;
>> + gimple fn_from;
>>
>> - print_generic_expr (stream, element->expr.ops.call.fn, 0);
>> + fn_from = element->expr.ops.call.fn_from;
>> + if (gimple_call_internal_p (fn_from))
>> + fputs (internal_fn_name (gimple_call_internal_fn (fn_from)),
>> + stream);
>> + else
>> + print_generic_expr (stream, gimple_call_fn (fn_from), 0);
>
> 2nd time, can you add a print_gimple_call_target () function instead?
OK.
>> Index: gcc/tree-ssa-sccvn.c
>> ===================================================================
>> --- gcc/tree-ssa-sccvn.c 2011-04-18 10:18:49.000000000 +0100
>> +++ gcc/tree-ssa-sccvn.c 2011-04-18 10:18:54.000000000 +0100
>> @@ -911,7 +911,10 @@ copy_reference_ops_from_call (gimple cal
>> memset (&temp, 0, sizeof (temp));
>> temp.type = gimple_call_return_type (call);
>> temp.opcode = CALL_EXPR;
>> - temp.op0 = gimple_call_fn (call);
>> + if (gimple_call_internal_p (call))
>> + temp.op0 = NULL_TREE;
>> + else
>> + temp.op0 = gimple_call_fn (call);
>
> That will now value-number all internal calls the same. A safe change
> would be to trivially value-number internal calls, like with
>
> Index: gcc/tree-ssa-sccvn.c
> ===================================================================
> --- gcc/tree-ssa-sccvn.c (revision 172640)
> +++ gcc/tree-ssa-sccvn.c (working copy)
> @@ -3125,7 +3125,8 @@ visit_use (tree use)
> /* ??? We should handle stores from calls. */
> else if (TREE_CODE (lhs) == SSA_NAME)
> {
> - if (gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST))
> + if (!gimple_call_internal_p (stmt)
> + && gimple_call_flags (stmt) & (ECF_PURE | ECF_CONST))
> changed = visit_reference_op_call (lhs, stmt);
> else
> changed = defs_to_varying (stmt);
>
> and a similar change in tree-ssa-pre.c:can_value_number_call.
We really should be able to treat calls to pure internal functions
like calls to pure "real" functions though.
TBH, I think this is an example of why trying to so hard to avoid a tree
code for the internal function is working against us. Most of the patch
feels like sprinkling hacks around the code base just because we don't
have a real tree representation for what we're calling. Any new code
that handles calls is going to have to remember to make the same
distinction.
The tree I suggested was an INTERNAL_FUNCTION node, with the appropriate
type and number attached. The nodes could be shared, rather than be
one-per-call. If we had that, then I think this, and a lot of the other
places I'm touching, would Just Work.
Richard
