diff mbox

Refactor gimple_expr_type

Message ID BLU179-W270C5862114E73737FEBABB6C50@phx.gbl
State New
Headers show

Commit Message

Aditya K May 17, 2015, 3:31 p.m. UTC
----------------------------------------
> 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.
gcc/ChangeLog:

2015-05-15  hiraditya  <hiraditya@msn.com>

        * gimple.h (gimple_expr_type): Refactor to make it concise. Remove redundant if.



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)
>>
>>

Comments

Richard Biener May 18, 2015, 10:08 a.m. UTC | #1
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,
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)
>>>
>>>
>
>
Aditya K May 18, 2015, 10:04 p.m. UTC | #2
----------------------------------------
> 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.

-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)
>>>>
>>>>
>>
>>
Richard Biener May 19, 2015, 9:33 a.m. UTC | #3
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)
>>>>>
>>>>>
>>>
>>>
>
Aditya K May 19, 2015, 4:50 p.m. UTC | #4
----------------------------------------
> Date: Tue, 19 May 2015 11:33:16 +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 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).

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 <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)
>>>>>>
>>>>>>
>>>>
>>>>
>>
Richard Biener May 20, 2015, 9:11 a.m. UTC | #5
On Tue, May 19, 2015 at 6:50 PM, Aditya K <hiraditya@msn.com> wrote:
>
>
> ----------------------------------------
>> Date: Tue, 19 May 2015 11:33:16 +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 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).
>
> 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.

Please re-send the patch as attachment, your mailer garbles the text
(send mails as non-unicode text/plain).

Richard.

> Thanks,
> -Aditya
>
>
>>
>> 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)
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>
>
>
Aditya K May 20, 2015, 1:29 p.m. UTC | #6
----------------------------------------
> Date: Wed, 20 May 2015 11:11:52 +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 Tue, May 19, 2015 at 6:50 PM, Aditya K <hiraditya@msn.com> wrote:
>>
>>
>> ----------------------------------------
>>> Date: Tue, 19 May 2015 11:33:16 +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 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).
>>
>> 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.
>
> Please re-send the patch as attachment, your mailer garbles the text
> (send mails as non-unicode text/plain).
>

Sure. I have attached the file.

Thanks,
-Aditya

> Richard.
>
>> Thanks,
>> -Aditya
>>
>>
>>>
>>> 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)
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>
>>
>>
diff mbox

Patch

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;