Re: [PATCH 1/2] Allow REG_EQUAL for ZERO_EXTRACT

[Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>]

> On Jun 30, 2015, at 6:54 AM, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
> 
> 
> On 29/06/15 21:56, Maxim Kuvyrkov wrote:
>>> On Jun 28, 2015, at 2:28 PM, Kugan <kugan.vivekanandarajah@linaro.org> wrote:
>>> 
>>> This patch allows setting REG_EQUAL for ZERO_EXTRACT and handle that in
>>> cse (where the src for the ZERO_EXTRACT needs to be calculated)
>>> 
>>> Thanks,
>>> Kugan
>> 
>>> From 75e746e559ffd21b25542b3db627e3b318118569 Mon Sep 17 00:00:00 2001
>>> From: Kugan Vivekanandarajah <kugan.vivekanandarajah@linaro.org>
>>> Date: Fri, 26 Jun 2015 17:12:07 +1000
>>> Subject: [PATCH 1/2] Allow adding REG_EQUAL for ZERO_EXTRACT
>>> 
>>> ---
>>> gcc/ChangeLog  |  6 ++++++
>>> gcc/cse.c      | 41 ++++++++++++++++++++++++++++++++++++++++-
>>> gcc/emit-rtl.c |  3 ++-
>>> 3 files changed, 48 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
>>> index 080aa39..d4a73d6 100644
>>> --- a/gcc/ChangeLog
>>> +++ b/gcc/ChangeLog
>>> @@ -1,3 +1,9 @@
>>> +2015-06-26  Kugan Vivekanandarajah  <kuganv@linaro.org>
>>> +
>>> +	* cse.c (cse_insn): Calculate src_eqv for ZERO_EXTRACT.
>>> +	* emit-rtl.c (set_for_reg_notes): Allow ZERO_EXTRACT to set
>>> +	REG_EQUAL note.
>>> +
>>> 2015-06-25  H.J. Lu  <hongjiu.lu@intel.com>
>>> 
>>> 	* gentarget-def.c (def_target_insn): Cast return of strtol to
>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>> index 100c9c8..8add651 100644
>>> --- a/gcc/cse.c
>>> +++ b/gcc/cse.c
>>> @@ -4531,8 +4531,47 @@ cse_insn (rtx_insn *insn)
>>>   if (n_sets == 1 && REG_NOTES (insn) != 0
>>>       && (tem = find_reg_note (insn, REG_EQUAL, NULL_RTX)) != 0
>>>       && (! rtx_equal_p (XEXP (tem, 0), SET_SRC (sets[0].rtl))
>>> +	  || GET_CODE (SET_DEST (sets[0].rtl)) == ZERO_EXTRACT
>>> 	  || GET_CODE (SET_DEST (sets[0].rtl)) == STRICT_LOW_PART))
>>> -    src_eqv = copy_rtx (XEXP (tem, 0));
>>> +    {
>>> +      src_eqv = copy_rtx (XEXP (tem, 0));
>>> +
>>> +      /* If DEST is of the form ZERO_EXTACT, as in:
>>> +	 (set (zero_extract:SI (reg:SI 119)
>>> +		  (const_int 16 [0x10])
>>> +		  (const_int 16 [0x10]))
>>> +	      (const_int 51154 [0xc7d2]))
>>> +	 REG_EQUAL note will specify the value of register (reg:SI 119) at this
>>> +	 point.  Note that this is different from SRC_EQV. We can however
>>> +	 calculate SRC_EQV with the position and width of ZERO_EXTRACT.  */
>>> +      if (GET_CODE (SET_DEST (sets[0].rtl)) == ZERO_EXTRACT)
>> 
>> Consider changing
>> 
>> if (something
>>    && (!rtx_equal_p)
>>        || ZERO_EXTRACT
>>        || STRICT_LOW_PART)
>> 
>> to 
>> 
>> if (something
>>    && !rtx_equal_p)
>>  {
>>     if (ZERO_EXTRACT)
>>       {
>>       }
>>     else if (STRICT_LOW_PART)
>>       {
>>       }
>>  }
>> 
>> Otherwise looks good to me, but you still need another approval.
> 
> Thanks Maxim for the review. How about the attached patch?

Looks good, with a couple of indentation nit-picks below.  No need to repost the patch on their account.  Wait for another a maintainer's review.

> --- a/gcc/cse.c
> +++ b/gcc/cse.c
> @@ -4525,14 +4525,49 @@ cse_insn (rtx_insn *insn)
>    canonicalize_insn (insn, &sets, n_sets);
>  
>    /* If this insn has a REG_EQUAL note, store the equivalent value in SRC_EQV,
> -     if different, or if the DEST is a STRICT_LOW_PART.  The latter condition
> -     is necessary because SRC_EQV is handled specially for this case, and if
> -     it isn't set, then there will be no equivalence for the destination.  */
> +     if different, or if the DEST is a STRICT_LOW_PART/ZERO_EXTRACT.  The
> +     latter condition is necessary because SRC_EQV is handled specially for
> +     this case, and if it isn't set, then there will be no equivalence
> +     for the destination.  */
>    if (n_sets == 1 && REG_NOTES (insn) != 0
> -      && (tem = find_reg_note (insn, REG_EQUAL, NULL_RTX)) != 0
> -      && (! rtx_equal_p (XEXP (tem, 0), SET_SRC (sets[0].rtl))
> -	  || GET_CODE (SET_DEST (sets[0].rtl)) == STRICT_LOW_PART))
> -    src_eqv = copy_rtx (XEXP (tem, 0));
> +      && (tem = find_reg_note (insn, REG_EQUAL, NULL_RTX)) != 0)
> +    {
> +      if ((! rtx_equal_p (XEXP (tem, 0), SET_SRC (sets[0].rtl)))
> +	  || GET_CODE (SET_DEST (sets[0].rtl)) == STRICT_LOW_PART)
> +      src_eqv = copy_rtx (XEXP (tem, 0));

Please double check indentation here.

> +
> +      /* If DEST is of the form ZERO_EXTACT, as in:
> +	 (set (zero_extract:SI (reg:SI 119)
> +		  (const_int 16 [0x10])
> +		  (const_int 16 [0x10]))
> +	      (const_int 51154 [0xc7d2]))
> +	 REG_EQUAL note will specify the value of register (reg:SI 119) at this
> +	 point.  Note that this is different from SRC_EQV. We can however
> +	 calculate SRC_EQV with the position and width of ZERO_EXTRACT.  */
> +      else if (GET_CODE (SET_DEST (sets[0].rtl)) == ZERO_EXTRACT
> +	       &&CONST_INT_P (src_eqv)

Add a space between && and CONST_INT_P.

> +	       && CONST_INT_P (XEXP (SET_DEST (sets[0].rtl), 1))
> +	       && CONST_INT_P (XEXP (SET_DEST (sets[0].rtl), 2)))
> +	{
> +	  rtx dest_reg = XEXP (SET_DEST (sets[0].rtl), 0);
> +	  rtx width = XEXP (SET_DEST (sets[0].rtl), 1);
> +	  rtx pos = XEXP (SET_DEST (sets[0].rtl), 2);
> +	  HOST_WIDE_INT val = INTVAL (src_eqv);
> +	  HOST_WIDE_INT mask;
> +	  unsigned int shift;
> +	  if (BITS_BIG_ENDIAN)
> +	    shift = GET_MODE_PRECISION (GET_MODE (dest_reg))
> +	      - INTVAL (pos) - INTVAL (width);

The usual practice is to brace the calculation that spans multiple lines, so that second and subsequent lines are aligned to the right of "=", e.g.,
shift = (GET_MODE_PRECISION (GET_MODE (dest_reg))
         - INTVAL (pos) - INTVAL (width));

> +	  else
> +	    shift = INTVAL (pos);
> +	  if (INTVAL (width) == HOST_BITS_PER_WIDE_INT)
> +	    mask = ~(HOST_WIDE_INT) 0;
> +	  else
> +	    mask = ((HOST_WIDE_INT) 1 << INTVAL (width)) - 1;
> +	  val = (val >> shift) & mask;
> +	  src_eqv = GEN_INT (val);
> +	}
> +    }
> 


--
Maxim Kuvyrkov
www.linaro.org