Richard Biener <richard.guent...@gmail.com> writes:
> On Tue, Nov 10, 2015 at 10:24 PM, Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>> Richard Biener <richard.guent...@gmail.com> writes:
>>> On Sat, Nov 7, 2015 at 2:31 PM, Richard Sandiford
>>> <richard.sandif...@arm.com> wrote:
>>>> This patch short-circuits the builtins.c expansion code for a particular
>>>> gimple call if:
>>>>
>>>> - the function has an associated internal function
>>>> - the target implements that internal function
>>>> - the call has no side effects
>>>>
>>>> This allows a later patch to remove the builtins.c code, once calls with
>>>> side effects have been handled.
>>>>
>>>> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-linux-gnueabi.
>>>> OK to install?
>>>>
>>>> Thanks,
>>>> Richard
>>>>
>>>>
>>>> gcc/
>>>> * builtins.h (called_as_built_in): Declare.
>>>> * builtins.c (called_as_built_in): Make external.
>>>> * internal-fn.h (expand_internal_call): Define a variant that
>>>> specifies the internal function explicitly.
>>>> * internal-fn.c (expand_load_lanes_optab_fn)
>>>> (expand_store_lanes_optab_fn, expand_ANNOTATE,
>>>> expand_GOMP_SIMD_LANE)
>>>> (expand_GOMP_SIMD_VF, expand_GOMP_SIMD_LAST_LANE)
>>>> (expand_GOMP_SIMD_ORDERED_START, expand_GOMP_SIMD_ORDERED_END)
>>>> (expand_UBSAN_NULL, expand_UBSAN_BOUNDS, expand_UBSAN_VPTR)
>>>> (expand_UBSAN_OBJECT_SIZE, expand_ASAN_CHECK,
>>>> expand_TSAN_FUNC_EXIT)
>>>> (expand_UBSAN_CHECK_ADD, expand_UBSAN_CHECK_SUB)
>>>> (expand_UBSAN_CHECK_MUL, expand_ADD_OVERFLOW, expand_SUB_OVERFLOW)
>>>> (expand_MUL_OVERFLOW, expand_LOOP_VECTORIZED)
>>>> (expand_mask_load_optab_fn, expand_mask_store_optab_fn)
>>>> (expand_ABNORMAL_DISPATCHER, expand_BUILTIN_EXPECT, expand_VA_ARG)
>>>> (expand_UNIQUE, expand_GOACC_DIM_SIZE, expand_GOACC_DIM_POS)
>>>> (expand_GOACC_LOOP, expand_GOACC_REDUCTION, expand_direct_optab_fn)
>>>> (expand_unary_optab_fn, expand_binary_optab_fn): Add an internal_fn
>>>> argument.
>>>> (internal_fn_expanders): Update prototype.
>>>> (expand_internal_call): Define a variant that specifies the
>>>> internal function explicitly. Use it to implement the previous
>>>> interface.
>>>> * cfgexpand.c (expand_call_stmt): Try to expand calls to built-in
>>>> functions as calls to internal functions.
>>>>
>>>> diff --git a/gcc/builtins.c b/gcc/builtins.c
>>>> index f65011e..bbcc7dc3 100644
>>>> --- a/gcc/builtins.c
>>>> +++ b/gcc/builtins.c
>>>> @@ -222,7 +222,7 @@ is_builtin_fn (tree decl)
>>>> of the optimization level. This means whenever a function is invoked
>>>> with
>>>> its "internal" name, which normally contains the prefix "__builtin".
>>>> */
>>>>
>>>> -static bool
>>>> +bool
>>>> called_as_built_in (tree node)
>>>> {
>>>> /* Note that we must use DECL_NAME, not DECL_ASSEMBLER_NAME_SET_P since
>>>> diff --git a/gcc/builtins.h b/gcc/builtins.h
>>>> index 917eb90..1d00068 100644
>>>> --- a/gcc/builtins.h
>>>> +++ b/gcc/builtins.h
>>>> @@ -50,6 +50,7 @@ extern struct target_builtins *this_target_builtins;
>>>> extern bool force_folding_builtin_constant_p;
>>>>
>>>> extern bool is_builtin_fn (tree);
>>>> +extern bool called_as_built_in (tree);
>>>> extern bool get_object_alignment_1 (tree, unsigned int *,
>>>> unsigned HOST_WIDE_INT *);
>>>> extern unsigned int get_object_alignment (tree);
>>>> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
>>>> index bfbc958..dc7d4f5 100644
>>>> --- a/gcc/cfgexpand.c
>>>> +++ b/gcc/cfgexpand.c
>>>> @@ -2551,10 +2551,25 @@ expand_call_stmt (gcall *stmt)
>>>> return;
>>>> }
>>>>
>>>> + /* If this is a call to a built-in function and it has no effect other
>>>> + than setting the lhs, try to implement it using an internal function
>>>> + instead. */
>>>> + decl = gimple_call_fndecl (stmt);
>>>> + if (gimple_call_lhs (stmt)
>>>> + && !gimple_vdef (stmt)
>>>
>>> I think you want && ! gimple_has_side_effects (stmt)
>>> instead of checking !gimple_vdef (stmt).
>>
>> OK, I can do that, but what would the difference be in practice for
>> these types of call? I.e. are there cases for built-ins where:
>>
>> (A) gimple_vdef (stmt) && !gimple_side_effects (stmt)
>>
>> or:
>>
>> (B) !gimple_vdef (stmt) && gimple_side_effects (stmt)
>>
>> ?
>
> There was talk to make calls use volatile to prevent CSE and friends.
>
> Using gimple_has_side_effects is just the better check.
>
>> It just seems like this check should be the opposite of the one used
>> in the call-cdce patch (when deciding whether to optimise a call
>> with an lhs). In order to keep them in sync I'd need to use
>> gimple_side_effects rather than gimple_vdef there too, but is
>> (B) a possibility there?
>
> Not sure if the tests should be in-sync.
Well, with the series, we have two opportunities to use optabs:
- in tree-call-cdce.c, if we need to keep the original call
(or something that sets EDOM)
- here in cfgexpand.c, if we don't need to keep the original call.
So both are really asking "do I need to keep the call?"
If we don't do either then we don't use the optab.
> I'm also not sure what you really want to check with
>
>>>> + /* If this is a call to a built-in function and it has no effect other
>>>> + than setting the lhs, try to implement it using an internal function
>>>> + instead. */
>>>> + decl = gimple_call_fndecl (stmt);
>>>> + if (gimple_call_lhs (stmt)
>>>> + && !gimple_vdef (stmt)
>
> I know you want to catch errno setting here but shouldn't that use a
> different kind of check and only done for a specific subset of
> functions? For example do you want to replace sqrt() with
> an IFN in case it has a VUSE (it will have that with -frounding-math)?
> In this case you'll lose the implicit dependence on fesetround and
> friends. This implicit dependence is not checked by gimple_has_side_effects
> either btw.
Yeah, we want to replace even in that case. Remember that this is
expand code, so we're going to generate rtl whatever happens. It's up
to the rtl patterns to represent any ordering requirements. (Not that
we do have explicit rtl dependencies for rounding mode, but we don't for
+, -. * or / either.)
If for some reason a particular .md pattern doesn't honour the current
rounding mode when normally the optab would be expected to, the .md file
should limit the pattern to !flag_rounding_math.
Thanks,
Richard
