RE: Refactor gimple_expr_type

[Aditya K <hiraditya@msn.com>]

[-- Attachment #1: Type: text/plain, Size: 9038 bytes --]



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

[-- Attachment #2: gimple_expr_type.patch --]
[-- Type: application/octet-stream, Size: 2278 bytes --]

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;