----------------------------------------
> Date: Tue, 19 May 2015 11:33:16 +0200
> Subject: Re: Refactor gimple_expr_type
> From: richard.guent...@gmail.com
> To: hiradi...@msn.com
> CC: tbsau...@tbsaunde.org; gcc-patches@gcc.gnu.org
>
> On Tue, May 19, 2015 at 12:04 AM, Aditya K <hiradi...@msn.com> wrote:
>>
>>
>> ----------------------------------------
>>> Date: Mon, 18 May 2015 12:08:58 +0200
>>> Subject: Re: Refactor gimple_expr_type
>>> From: richard.guent...@gmail.com
>>> To: hiradi...@msn.com
>>> CC: tbsau...@tbsaunde.org; gcc-patches@gcc.gnu.org
>>>
>>> On Sun, May 17, 2015 at 5:31 PM, Aditya K <hiradi...@msn.com> wrote:
>>>>
>>>>
>>>> ----------------------------------------
>>>>> Date: Sat, 16 May 2015 11:53:57 -0400
>>>>> From: tbsau...@tbsaunde.org
>>>>> To: hiradi...@msn.com
>>>>> CC: gcc-patches@gcc.gnu.org
>>>>> Subject: Re: Refactor gimple_expr_type
>>>>>
>>>>> On Fri, May 15, 2015 at 07:13:35AM +0000, Aditya K wrote:
>>>>>> Hi,
>>>>>> I have tried to refactor gimple_expr_type to make it more readable.
>>>>>> Removed the switch block and redundant if.
>>>>>>
>>>>>> Please review this patch.
>>>>>
>>>>> for some reason your mail client seems to be inserting non breaking
>>>>> spaces all over the place. Please either configure it to not do that,
>>>>> or use git send-email for patches.
>>>>
>>>> Please see the updated patch.
>>>
>>> Ok if this passed bootstrap and regtest. (I wish if gimple_expr_type
>>> didn't exist btw...)
>>
>> Thanks for the review. Do you have any suggestions on how to remove
>> gimple_expr_type. Are there any alternatives to it?
>> I can look into refactoring more (if it is not too complicated) since I'm
>> already doing this.
>
> Look at each caller - usually they should be fine with using TREE_TYPE
> (gimple_get_lhs ()) (or a more specific one
> dependent on what stmts are expected at the place). You might want to
> first refactor the code
>
> else if (code == GIMPLE_COND)
> gcc_unreachable ();
>
> and deal with the fallout in callers (similar for the void_type_node return).
Thanks for the suggestions. I looked at the use cases there are 47 usages in
different files. That might be a lot of changes I assume, and would take some
time.
This patch passes bootstrap and make check (although I'm not very confident
that my way of make check ran all the regtests)
If this patch is okay to merge please do that. I'll continue working on
removing gimle_expr_type.
Thanks,
-Aditya
>
> Richard.
>
>
>> -Aditya
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> gcc/ChangeLog:
>>>>
>>>> 2015-05-15 hiraditya <hiradi...@msn.com>
>>>>
>>>> * gimple.h (gimple_expr_type): Refactor to make it concise. Remove
>>>> redundant if.
>>>>
>>>> diff --git a/gcc/gimple.h b/gcc/gimple.h
>>>> index 95e4fc8..3a83e8f 100644
>>>> --- a/gcc/gimple.h
>>>> +++ b/gcc/gimple.h
>>>> @@ -5717,36 +5717,26 @@ static inline tree
>>>> gimple_expr_type (const_gimple stmt)
>>>> {
>>>> enum gimple_code code = gimple_code (stmt);
>>>> -
>>>> - if (code == GIMPLE_ASSIGN || code == GIMPLE_CALL)
>>>> + /* In general we want to pass out a type that can be substituted
>>>> + for both the RHS and the LHS types if there is a possibly
>>>> + useless conversion involved. That means returning the
>>>> + original RHS type as far as we can reconstruct it. */
>>>> + if (code == GIMPLE_CALL)
>>>> {
>>>> - tree type;
>>>> - /* In general we want to pass out a type that can be substituted
>>>> - for both the RHS and the LHS types if there is a possibly
>>>> - useless conversion involved. That means returning the
>>>> - original RHS type as far as we can reconstruct it. */
>>>> - if (code == GIMPLE_CALL)
>>>> - {
>>>> - const gcall *call_stmt = as_a <const gcall *> (stmt);
>>>> - if (gimple_call_internal_p (call_stmt)
>>>> - && gimple_call_internal_fn (call_stmt) == IFN_MASK_STORE)
>>>> - type = TREE_TYPE (gimple_call_arg (call_stmt, 3));
>>>> - else
>>>> - type = gimple_call_return_type (call_stmt);
>>>> - }
>>>> + const gcall *call_stmt = as_a <const gcall *> (stmt);
>>>> + if (gimple_call_internal_p (call_stmt)
>>>> + && gimple_call_internal_fn (call_stmt) == IFN_MASK_STORE)
>>>> + return TREE_TYPE (gimple_call_arg (call_stmt, 3));
>>>> + else
>>>> + return gimple_call_return_type (call_stmt);
>>>> + }
>>>> + else if (code == GIMPLE_ASSIGN)
>>>> + {
>>>> + if (gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR)
>>>> + return TREE_TYPE (gimple_assign_rhs1 (stmt));
>>>> else
>>>> - switch (gimple_assign_rhs_code (stmt))
>>>> - {
>>>> - case POINTER_PLUS_EXPR:
>>>> - type = TREE_TYPE (gimple_assign_rhs1 (stmt));
>>>> - break;
>>>> -
>>>> - default:
>>>> - /* As fallback use the type of the LHS. */
>>>> - type = TREE_TYPE (gimple_get_lhs (stmt));
>>>> - break;
>>>> - }
>>>> - return type;
>>>> + /* As fallback use the type of the LHS. */
>>>> + return TREE_TYPE (gimple_get_lhs (stmt));
>>>> }
>>>> else if (code == GIMPLE_COND)
>>>> return boolean_type_node;
>>>>
>>>>
>>>> Thanks,
>>>> -Aditya
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> -Aditya
>>>>>>
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2015-05-15 hiraditya <hiradi...@msn.com>
>>>>>>
>>>>>> * gimple.h (gimple_expr_type): Refactor to make it concise. Remove
>>>>>> redundant if.
>>>>>>
>>>>>> diff --git a/gcc/gimple.h b/gcc/gimple.h
>>>>>> index 95e4fc8..168d3ba 100644
>>>>>> --- a/gcc/gimple.h
>>>>>> +++ b/gcc/gimple.h
>>>>>> @@ -5717,35 +5717,28 @@ static inline tree
>>>>>> gimple_expr_type (const_gimple stmt)
>>>>>> {
>>>>>> enum gimple_code code = gimple_code (stmt);
>>>>>> -
>>>>>> - if (code == GIMPLE_ASSIGN || code == GIMPLE_CALL)
>>>>>> + tree type;
>>>>>> + /* In general we want to pass out a type that can be substituted
>>>>>> + for both the RHS and the LHS types if there is a possibly
>>>>>> + useless conversion involved. That means returning the
>>>>>> + original RHS type as far as we can reconstruct it. */
>>>>>> + if (code == GIMPLE_CALL)
>>>>>> {
>>>>>> - tree type;
>>>>>> - /* In general we want to pass out a type that can be substituted
>>>>>> - for both the RHS and the LHS types if there is a possibly
>>>>>> - useless conversion involved. That means returning the
>>>>>> - original RHS type as far as we can reconstruct it. */
>>>>>> - if (code == GIMPLE_CALL)
>>>>>> - {
>>>>>> - const gcall *call_stmt = as_a <const gcall *> (stmt);
>>>>>> - if (gimple_call_internal_p (call_stmt)
>>>>>> - && gimple_call_internal_fn (call_stmt) == IFN_MASK_STORE)
>>>>>> - type = TREE_TYPE (gimple_call_arg (call_stmt, 3));
>>>>>> - else
>>>>>> - type = gimple_call_return_type (call_stmt);
>>>>>> - }
>>>>>> + const gcall *call_stmt = as_a <const gcall *> (stmt);
>>>>>> + if (gimple_call_internal_p (call_stmt)
>>>>>> + && gimple_call_internal_fn (call_stmt) == IFN_MASK_STORE)
>>>>>> + type = TREE_TYPE (gimple_call_arg (call_stmt, 3));
>>>>>> + else
>>>>>> + type = gimple_call_return_type (call_stmt);
>>>>>> + return type;
>>>>>
>>>>> You might as well return the value directly and not use the variable.
>>>>>
>>>>>> + }
>>>>>> + else if (code == GIMPLE_ASSIGN)
>>>>>
>>>>> else after return is kind of silly.
>>>>>
>>>>>> + {
>>>>>> + if (gimple_assign_rhs_code (stmt) == POINTER_PLUS_EXPR)
>>>>>> + type = TREE_TYPE (gimple_assign_rhs1 (stmt));
>>>>>> else
>>>>>> - switch (gimple_assign_rhs_code (stmt))
>>>>>> - {
>>>>>> - case POINTER_PLUS_EXPR:
>>>>>> - type = TREE_TYPE (gimple_assign_rhs1 (stmt));
>>>>>> - break;
>>>>>> -
>>>>>> - default:
>>>>>> - /* As fallback use the type of the LHS. */
>>>>>> - type = TREE_TYPE (gimple_get_lhs (stmt));
>>>>>> - break;
>>>>>> - }
>>>>>> + /* As fallback use the type of the LHS. */
>>>>>> + type = TREE_TYPE (gimple_get_lhs (stmt));
>>>>>> return type;
>>>>>
>>>>> might as well not use type here either.
>>>>>
>>>>> Trev
>>>>>
>>>>>> }
>>>>>> else if (code == GIMPLE_COND)
>>>>>>
>>>>>>
>>>>
>>>>
>>
