Re: Refactor gimple_expr_type

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, May 19, 2015 at 12:04 AM, Aditya K <hiraditya@msn.com> wrote:
>
>
> ----------------------------------------
>> Date: Mon, 18 May 2015 12:08:58 +0200
>> Subject: Re: Refactor gimple_expr_type
>> From: richard.guenther@gmail.com
>> To: hiraditya@msn.com
>> CC: tbsaunde@tbsaunde.org; gcc-patches@gcc.gnu.org
>>
>> On Sun, May 17, 2015 at 5:31 PM, Aditya K <hiraditya@msn.com> wrote:
>>>
>>>
>>> ----------------------------------------
>>>> Date: Sat, 16 May 2015 11:53:57 -0400
>>>> From: tbsaunde@tbsaunde.org
>>>> To: hiraditya@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).

Richard.


> -Aditya
>
>>
>> Thanks,
>> Richard.
>>
>>> gcc/ChangeLog:
>>>
>>> 2015-05-15 hiraditya <hiraditya@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 <hiraditya@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)
>>>>>
>>>>>
>>>
>>>
>