Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Markus Trippelsdorf]

On 2016.06.28 at 11:21 -0700, Gary Funck wrote:
> On 06/20/16 04:55:16, H.J. Lu wrote:
> > TImode register referenced in debug insn can be converted to V1TImode
> > by scalar to vector optimization.  We need to convert a debug insn if
> > it has a variable in a TImode register.
> 
> We have a situation on a few of the UPC tests, where they ICE on
> this gcc_assert().
> 
> 3820  gcc_assert (REG_P (loc)
> 3821  && GET_MODE (loc) == V1TImode);
> 
> (gdb) p val
> $2 = (rtx) 0x7fffef6d3978
> (gdb) pr
> warning: Expression is not an assignment (and might have no effect)
> (var_location:TI newval (subreg:TI (reg/v/f:V1TI 307 [ newval ]) 0))
> 
> (gdb) p loc
> $1 = (rtx) 0x7fffef409210
> (gdb) pr
> warning: Expression is not an assignment (and might have no effect)
> (subreg:TI (reg/v/f:V1TI 307 [ newval ]) 0)
> 
> As you can see, 'loc' is already a TI mode subreg based upon a V1TI mode reg.
> 
> I didn't try tracking down how we end up with 'loc' as a subreg, but will
> note that in UPC the pointer-to-shared representation is a 16 byte struct,
> aligned on 16 bytes, so the generated code will frequently deal with TImode
> values in registers.
> 
> Given the code that follows this assert,
> 
> 3822  /* Convert V1TImode register, which has been updated by a 
> SET
> 3823 insn before, to SUBREG TImode.  */
> 3824  PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 
> 0);
> 3825  df_insn_rescan (insn);
> 
> converts the V1TImode register into a TImode subreg, and we already have
> that situation, I tried the following patch:
> 
> --- /a/gcc-trunk/gcc/config/i386/i386.c 2016-06-26 19:01:12.099740515 -0700
> +++ config/i386/i386.c  2016-06-28 11:17:26.323396045 -0700
> @@ -3814,6 +3814,9 @@
> continue;
>   gcc_assert (GET_CODE (val) == VAR_LOCATION);
>   rtx loc = PAT_VAR_LOCATION_LOC (val);
> + /* If already a SUBREG, skip.  */
> + if (SUBREG_P (loc))
> +   continue;
>   gcc_assert (REG_P (loc)
>   && GET_MODE (loc) == V1TImode);
>   /* Convert V1TImode register, which has been updated by a SET
> 
> 
> Can the patch be amended to include this fix?  Let me know if you need
> additional information, or would like me to try something else.

See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71801, which has a
small testcase, that is fixed by Gary's patch, too.

-- 
Markus

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Gary Funck]

On 06/20/16 04:55:16, H.J. Lu wrote:
> TImode register referenced in debug insn can be converted to V1TImode
> by scalar to vector optimization.  We need to convert a debug insn if
> it has a variable in a TImode register.

Hello,

We have a situation on a few of the UPC tests, where they ICE on
this gcc_assert().

3820  gcc_assert (REG_P (loc)
3821  && GET_MODE (loc) == V1TImode);

(gdb) p val
$2 = (rtx) 0x7fffef6d3978
(gdb) pr
warning: Expression is not an assignment (and might have no effect)
(var_location:TI newval (subreg:TI (reg/v/f:V1TI 307 [ newval ]) 0))

(gdb) p loc
$1 = (rtx) 0x7fffef409210
(gdb) pr
warning: Expression is not an assignment (and might have no effect)
(subreg:TI (reg/v/f:V1TI 307 [ newval ]) 0)

As you can see, 'loc' is already a TI mode subreg based upon a V1TI mode reg.

I didn't try tracking down how we end up with 'loc' as a subreg, but will
note that in UPC the pointer-to-shared representation is a 16 byte struct,
aligned on 16 bytes, so the generated code will frequently deal with TImode
values in registers.

Given the code that follows this assert,

3822  /* Convert V1TImode register, which has been updated by a SET
3823 insn before, to SUBREG TImode.  */
3824  PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
3825  df_insn_rescan (insn);

converts the V1TImode register into a TImode subreg, and we already have
that situation, I tried the following patch:

--- /a/gcc-trunk/gcc/config/i386/i386.c 2016-06-26 19:01:12.099740515 -0700
+++ config/i386/i386.c  2016-06-28 11:17:26.323396045 -0700
@@ -3814,6 +3814,9 @@
continue;
  gcc_assert (GET_CODE (val) == VAR_LOCATION);
  rtx loc = PAT_VAR_LOCATION_LOC (val);
+ /* If already a SUBREG, skip.  */
+ if (SUBREG_P (loc))
+   continue;
  gcc_assert (REG_P (loc)
  && GET_MODE (loc) == V1TImode);
  /* Convert V1TImode register, which has been updated by a SET


Can the patch be amended to include this fix?  Let me know if you need
additional information, or would like me to try something else.

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Ilya Enkovich]

2016-06-21 15:48 GMT+03:00 H.J. Lu :
> On Mon, Jun 20, 2016 at 11:53 AM, H.J. Lu  wrote:
>> On Mon, Jun 20, 2016 at 10:31 AM, Ilya Enkovich  
>> wrote:
>>> On 20 Jun 09:45, H.J. Lu wrote:
 On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich  
 wrote:
 > 2016-06-20 16:39 GMT+03:00 Uros Bizjak :
 >> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
 >>> TImode register referenced in debug insn can be converted to V1TImode
 >>> by scalar to vector optimization.  We need to convert a debug insn if
 >>> it has a variable in a TImode register.
 >>>
 >>> Tested on x86-64.  OK for trunk?
 >>
 >> Ilya, does this approach look good to you? Also, does DImode STV
 >> conversion need similar handling of debug insns?
 >
 > DImode conversion doesn't change register mode (i.e. never calls
 > PUT_MODE for registers).  That keeps debug instructions valid.
 >
 > Overall I don't like the idea of having debug insns in candidates
 > set and in chains.  Looks like it is possible to have a chain
 > consisting of a debug insn only which is weird (otherwise I don't
 > see why we may return false in timode_scalar_chain::convert_insn).

 Yes, it can happen:

 (insn 11 8 12 2 (parallel [
 (set (reg/v:TI 91 [  ])
 (plus:TI (reg/v:TI 92 [ a ])
 (reg/v:TI 96 [ b ])))
 (clobber (reg:CC 17 flags))
 ]) y.i:5 210 {*addti3_doubleword}
  (expr_list:REG_UNUSED (reg:CC 17 flags)
 (nil)))
 (debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [  ])) 
 y.i:5 -1
  (nil))


 > What about other possible register uses?  If debug insns are added
 > to candidates then NONDEBUG_INSN_P check for uses in
 > timode_check_non_convertible_regs becomes invalid, right?

 Debug insn has no impact on STV decision.  We just need to convert
 register referenced in debug insn from V1TImode to TImode in
 timode_scalar_chain::convert_insn.

 > If we have (or want) to fix some register uses then it's probably
 > would be better to visit register uses when we convert its mode
 > and make required fix-ups.  It seems better to me to not involve
 > debug insns in analysis phase.

 Here is the updated patch to add debug insn, which references the
 TImode register which will be converted to V1TImode to queue.
 I am testing it now.

>>>
>>> You still count and dump debug insns as optimized ones.  Also we
>>> try to use virtual functions to cover differences in DI and TI
>>> optimizations and introducing additional TARGET_64BIT in common
>>> STV code is undesirable.
>>>
>>> Also your conversion now depends on instructions processing order.
>>> You will fail to process debug insn before non-debug ones. Required
>>> order is not guaranteed because processing depends on instruction
>>> UIDs only.
>>>
>>> I propose to modify transformation phase only like in the patch
>>> (untested) below.  I rely on your code which assumes the only
>>> possible usage in debug insn is VAR_LOCATION.
>>>
>>> Thanks,
>>> Ilya
>>> --
>>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>>> index c5e5e12..ec955f0 100644
>>> --- a/gcc/config/i386/i386.c
>>> +++ b/gcc/config/i386/i386.c
>>> @@ -3139,6 +3139,7 @@ class timode_scalar_chain : public scalar_chain
>>>
>>>   private:
>>>void mark_dual_mode_def (df_ref def);
>>> +  void fix_debug_reg_uses (rtx reg);
>>>void convert_insn (rtx_insn *insn);
>>>/* We don't convert registers to difference size.  */
>>>void convert_registers () {}
>>> @@ -3790,6 +3791,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
>>>df_insn_rescan (insn);
>>>  }
>>>
>>> +/* Fix uses of converted REG in debug insns.  */
>>> +
>>> +void
>>> +timode_scalar_chain::fix_debug_reg_uses (rtx reg)
>>> +{
>>> +  df_ref ref;
>>> +  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = DF_REF_NEXT_REG 
>>> (ref))
>>> +{
>>> +  rtx_insn *insn = DF_REF_INSN (ref);
>>> +
>>> +  if (DEBUG_INSN_P (insn))
>>> +   {
>>> + /* It must be a debug insn with a TImode variable in register.  */
>>> + rtx val = PATTERN (insn);
>>> + gcc_assert (GET_MODE (val) == TImode
>>> + && GET_CODE (val) == VAR_LOCATION);
>>> + rtx loc = PAT_VAR_LOCATION_LOC (val);
>>> + gcc_assert (REG_P (loc)
>>> + && GET_MODE (loc) == V1TImode);
>>> + /* Convert V1TImode register, which has been updated by a SET
>>> + insn before, to SUBREG TImode.  */
>>> + PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
>>> + df_insn_rescan (insn);
>>> + return;
>>> +   }
>>> +}
>>> +}
>>> +
>>>  /* Convert INSN from TImode to 

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[H.J. Lu]

On Mon, Jun 20, 2016 at 11:53 AM, H.J. Lu  wrote:
> On Mon, Jun 20, 2016 at 10:31 AM, Ilya Enkovich  
> wrote:
>> On 20 Jun 09:45, H.J. Lu wrote:
>>> On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich  
>>> wrote:
>>> > 2016-06-20 16:39 GMT+03:00 Uros Bizjak :
>>> >> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
>>> >>> TImode register referenced in debug insn can be converted to V1TImode
>>> >>> by scalar to vector optimization.  We need to convert a debug insn if
>>> >>> it has a variable in a TImode register.
>>> >>>
>>> >>> Tested on x86-64.  OK for trunk?
>>> >>
>>> >> Ilya, does this approach look good to you? Also, does DImode STV
>>> >> conversion need similar handling of debug insns?
>>> >
>>> > DImode conversion doesn't change register mode (i.e. never calls
>>> > PUT_MODE for registers).  That keeps debug instructions valid.
>>> >
>>> > Overall I don't like the idea of having debug insns in candidates
>>> > set and in chains.  Looks like it is possible to have a chain
>>> > consisting of a debug insn only which is weird (otherwise I don't
>>> > see why we may return false in timode_scalar_chain::convert_insn).
>>>
>>> Yes, it can happen:
>>>
>>> (insn 11 8 12 2 (parallel [
>>> (set (reg/v:TI 91 [  ])
>>> (plus:TI (reg/v:TI 92 [ a ])
>>> (reg/v:TI 96 [ b ])))
>>> (clobber (reg:CC 17 flags))
>>> ]) y.i:5 210 {*addti3_doubleword}
>>>  (expr_list:REG_UNUSED (reg:CC 17 flags)
>>> (nil)))
>>> (debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [  ])) y.i:5 
>>> -1
>>>  (nil))
>>>
>>>
>>> > What about other possible register uses?  If debug insns are added
>>> > to candidates then NONDEBUG_INSN_P check for uses in
>>> > timode_check_non_convertible_regs becomes invalid, right?
>>>
>>> Debug insn has no impact on STV decision.  We just need to convert
>>> register referenced in debug insn from V1TImode to TImode in
>>> timode_scalar_chain::convert_insn.
>>>
>>> > If we have (or want) to fix some register uses then it's probably
>>> > would be better to visit register uses when we convert its mode
>>> > and make required fix-ups.  It seems better to me to not involve
>>> > debug insns in analysis phase.
>>>
>>> Here is the updated patch to add debug insn, which references the
>>> TImode register which will be converted to V1TImode to queue.
>>> I am testing it now.
>>>
>>
>> You still count and dump debug insns as optimized ones.  Also we
>> try to use virtual functions to cover differences in DI and TI
>> optimizations and introducing additional TARGET_64BIT in common
>> STV code is undesirable.
>>
>> Also your conversion now depends on instructions processing order.
>> You will fail to process debug insn before non-debug ones. Required
>> order is not guaranteed because processing depends on instruction
>> UIDs only.
>>
>> I propose to modify transformation phase only like in the patch
>> (untested) below.  I rely on your code which assumes the only
>> possible usage in debug insn is VAR_LOCATION.
>>
>> Thanks,
>> Ilya
>> --
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index c5e5e12..ec955f0 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -3139,6 +3139,7 @@ class timode_scalar_chain : public scalar_chain
>>
>>   private:
>>void mark_dual_mode_def (df_ref def);
>> +  void fix_debug_reg_uses (rtx reg);
>>void convert_insn (rtx_insn *insn);
>>/* We don't convert registers to difference size.  */
>>void convert_registers () {}
>> @@ -3790,6 +3791,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
>>df_insn_rescan (insn);
>>  }
>>
>> +/* Fix uses of converted REG in debug insns.  */
>> +
>> +void
>> +timode_scalar_chain::fix_debug_reg_uses (rtx reg)
>> +{
>> +  df_ref ref;
>> +  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = DF_REF_NEXT_REG 
>> (ref))
>> +{
>> +  rtx_insn *insn = DF_REF_INSN (ref);
>> +
>> +  if (DEBUG_INSN_P (insn))
>> +   {
>> + /* It must be a debug insn with a TImode variable in register.  */
>> + rtx val = PATTERN (insn);
>> + gcc_assert (GET_MODE (val) == TImode
>> + && GET_CODE (val) == VAR_LOCATION);
>> + rtx loc = PAT_VAR_LOCATION_LOC (val);
>> + gcc_assert (REG_P (loc)
>> + && GET_MODE (loc) == V1TImode);
>> + /* Convert V1TImode register, which has been updated by a SET
>> + insn before, to SUBREG TImode.  */
>> + PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
>> + df_insn_rescan (insn);
>> + return;
>> +   }
>> +}
>> +}
>> +
>>  /* Convert INSN from TImode to V1T1mode.  */
>>
>>  void
>> @@ -3806,8 +3835,10 @@ timode_scalar_chain::convert_insn (rtx_insn *insn)
>> rtx tmp = find_reg_equal_equiv_note (insn);
>> if (tmp)
>>

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[H.J. Lu]

On Mon, Jun 20, 2016 at 10:31 AM, Ilya Enkovich  wrote:
> On 20 Jun 09:45, H.J. Lu wrote:
>> On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich  
>> wrote:
>> > 2016-06-20 16:39 GMT+03:00 Uros Bizjak :
>> >> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
>> >>> TImode register referenced in debug insn can be converted to V1TImode
>> >>> by scalar to vector optimization.  We need to convert a debug insn if
>> >>> it has a variable in a TImode register.
>> >>>
>> >>> Tested on x86-64.  OK for trunk?
>> >>
>> >> Ilya, does this approach look good to you? Also, does DImode STV
>> >> conversion need similar handling of debug insns?
>> >
>> > DImode conversion doesn't change register mode (i.e. never calls
>> > PUT_MODE for registers).  That keeps debug instructions valid.
>> >
>> > Overall I don't like the idea of having debug insns in candidates
>> > set and in chains.  Looks like it is possible to have a chain
>> > consisting of a debug insn only which is weird (otherwise I don't
>> > see why we may return false in timode_scalar_chain::convert_insn).
>>
>> Yes, it can happen:
>>
>> (insn 11 8 12 2 (parallel [
>> (set (reg/v:TI 91 [  ])
>> (plus:TI (reg/v:TI 92 [ a ])
>> (reg/v:TI 96 [ b ])))
>> (clobber (reg:CC 17 flags))
>> ]) y.i:5 210 {*addti3_doubleword}
>>  (expr_list:REG_UNUSED (reg:CC 17 flags)
>> (nil)))
>> (debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [  ])) y.i:5 
>> -1
>>  (nil))
>>
>>
>> > What about other possible register uses?  If debug insns are added
>> > to candidates then NONDEBUG_INSN_P check for uses in
>> > timode_check_non_convertible_regs becomes invalid, right?
>>
>> Debug insn has no impact on STV decision.  We just need to convert
>> register referenced in debug insn from V1TImode to TImode in
>> timode_scalar_chain::convert_insn.
>>
>> > If we have (or want) to fix some register uses then it's probably
>> > would be better to visit register uses when we convert its mode
>> > and make required fix-ups.  It seems better to me to not involve
>> > debug insns in analysis phase.
>>
>> Here is the updated patch to add debug insn, which references the
>> TImode register which will be converted to V1TImode to queue.
>> I am testing it now.
>>
>
> You still count and dump debug insns as optimized ones.  Also we
> try to use virtual functions to cover differences in DI and TI
> optimizations and introducing additional TARGET_64BIT in common
> STV code is undesirable.
>
> Also your conversion now depends on instructions processing order.
> You will fail to process debug insn before non-debug ones. Required
> order is not guaranteed because processing depends on instruction
> UIDs only.
>
> I propose to modify transformation phase only like in the patch
> (untested) below.  I rely on your code which assumes the only
> possible usage in debug insn is VAR_LOCATION.
>
> Thanks,
> Ilya
> --
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index c5e5e12..ec955f0 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -3139,6 +3139,7 @@ class timode_scalar_chain : public scalar_chain
>
>   private:
>void mark_dual_mode_def (df_ref def);
> +  void fix_debug_reg_uses (rtx reg);
>void convert_insn (rtx_insn *insn);
>/* We don't convert registers to difference size.  */
>void convert_registers () {}
> @@ -3790,6 +3791,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
>df_insn_rescan (insn);
>  }
>
> +/* Fix uses of converted REG in debug insns.  */
> +
> +void
> +timode_scalar_chain::fix_debug_reg_uses (rtx reg)
> +{
> +  df_ref ref;
> +  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = DF_REF_NEXT_REG 
> (ref))
> +{
> +  rtx_insn *insn = DF_REF_INSN (ref);
> +
> +  if (DEBUG_INSN_P (insn))
> +   {
> + /* It must be a debug insn with a TImode variable in register.  */
> + rtx val = PATTERN (insn);
> + gcc_assert (GET_MODE (val) == TImode
> + && GET_CODE (val) == VAR_LOCATION);
> + rtx loc = PAT_VAR_LOCATION_LOC (val);
> + gcc_assert (REG_P (loc)
> + && GET_MODE (loc) == V1TImode);
> + /* Convert V1TImode register, which has been updated by a SET
> + insn before, to SUBREG TImode.  */
> + PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
> + df_insn_rescan (insn);
> + return;
> +   }
> +}
> +}
> +
>  /* Convert INSN from TImode to V1T1mode.  */
>
>  void
> @@ -3806,8 +3835,10 @@ timode_scalar_chain::convert_insn (rtx_insn *insn)
> rtx tmp = find_reg_equal_equiv_note (insn);
> if (tmp)
>   PUT_MODE (XEXP (tmp, 0), V1TImode);
> +   PUT_MODE (dst, V1TImode);
> +   fix_debug_reg_uses (dst);
>}
> -  /* FALLTHRU */
> +  break;
>  case MEM:
>PUT_MODE 

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Ilya Enkovich]

On 20 Jun 09:45, H.J. Lu wrote:
> On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich  wrote:
> > 2016-06-20 16:39 GMT+03:00 Uros Bizjak :
> >> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
> >>> TImode register referenced in debug insn can be converted to V1TImode
> >>> by scalar to vector optimization.  We need to convert a debug insn if
> >>> it has a variable in a TImode register.
> >>>
> >>> Tested on x86-64.  OK for trunk?
> >>
> >> Ilya, does this approach look good to you? Also, does DImode STV
> >> conversion need similar handling of debug insns?
> >
> > DImode conversion doesn't change register mode (i.e. never calls
> > PUT_MODE for registers).  That keeps debug instructions valid.
> >
> > Overall I don't like the idea of having debug insns in candidates
> > set and in chains.  Looks like it is possible to have a chain
> > consisting of a debug insn only which is weird (otherwise I don't
> > see why we may return false in timode_scalar_chain::convert_insn).
> 
> Yes, it can happen:
> 
> (insn 11 8 12 2 (parallel [
> (set (reg/v:TI 91 [  ])
> (plus:TI (reg/v:TI 92 [ a ])
> (reg/v:TI 96 [ b ])))
> (clobber (reg:CC 17 flags))
> ]) y.i:5 210 {*addti3_doubleword}
>  (expr_list:REG_UNUSED (reg:CC 17 flags)
> (nil)))
> (debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [  ])) y.i:5 -1
>  (nil))
> 
> 
> > What about other possible register uses?  If debug insns are added
> > to candidates then NONDEBUG_INSN_P check for uses in
> > timode_check_non_convertible_regs becomes invalid, right?
> 
> Debug insn has no impact on STV decision.  We just need to convert
> register referenced in debug insn from V1TImode to TImode in
> timode_scalar_chain::convert_insn.
> 
> > If we have (or want) to fix some register uses then it's probably
> > would be better to visit register uses when we convert its mode
> > and make required fix-ups.  It seems better to me to not involve
> > debug insns in analysis phase.
> 
> Here is the updated patch to add debug insn, which references the
> TImode register which will be converted to V1TImode to queue.
> I am testing it now.
> 

You still count and dump debug insns as optimized ones.  Also we
try to use virtual functions to cover differences in DI and TI 
optimizations and introducing additional TARGET_64BIT in common
STV code is undesirable.

Also your conversion now depends on instructions processing order.
You will fail to process debug insn before non-debug ones. Required
order is not guaranteed because processing depends on instruction
UIDs only.

I propose to modify transformation phase only like in the patch
(untested) below.  I rely on your code which assumes the only
possible usage in debug insn is VAR_LOCATION.

Thanks,
Ilya
--
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c5e5e12..ec955f0 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -3139,6 +3139,7 @@ class timode_scalar_chain : public scalar_chain
 
  private:
   void mark_dual_mode_def (df_ref def);
+  void fix_debug_reg_uses (rtx reg);
   void convert_insn (rtx_insn *insn);
   /* We don't convert registers to difference size.  */
   void convert_registers () {}
@@ -3790,6 +3791,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
   df_insn_rescan (insn);
 }
 
+/* Fix uses of converted REG in debug insns.  */
+
+void
+timode_scalar_chain::fix_debug_reg_uses (rtx reg)
+{
+  df_ref ref;
+  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = DF_REF_NEXT_REG (ref))
+{
+  rtx_insn *insn = DF_REF_INSN (ref);
+
+  if (DEBUG_INSN_P (insn))
+   {
+ /* It must be a debug insn with a TImode variable in register.  */
+ rtx val = PATTERN (insn);
+ gcc_assert (GET_MODE (val) == TImode
+ && GET_CODE (val) == VAR_LOCATION);
+ rtx loc = PAT_VAR_LOCATION_LOC (val);
+ gcc_assert (REG_P (loc)
+ && GET_MODE (loc) == V1TImode);
+ /* Convert V1TImode register, which has been updated by a SET
+ insn before, to SUBREG TImode.  */
+ PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
+ df_insn_rescan (insn);
+ return;
+   }
+}
+}
+
 /* Convert INSN from TImode to V1T1mode.  */
 
 void
@@ -3806,8 +3835,10 @@ timode_scalar_chain::convert_insn (rtx_insn *insn)
rtx tmp = find_reg_equal_equiv_note (insn);
if (tmp)
  PUT_MODE (XEXP (tmp, 0), V1TImode);
+   PUT_MODE (dst, V1TImode);
+   fix_debug_reg_uses (dst);
   }
-  /* FALLTHRU */
+  break;
 case MEM:
   PUT_MODE (dst, V1TImode);
   break;

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[H.J. Lu]

On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich  wrote:
> 2016-06-20 16:39 GMT+03:00 Uros Bizjak :
>> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
>>> TImode register referenced in debug insn can be converted to V1TImode
>>> by scalar to vector optimization.  We need to convert a debug insn if
>>> it has a variable in a TImode register.
>>>
>>> Tested on x86-64.  OK for trunk?
>>
>> Ilya, does this approach look good to you? Also, does DImode STV
>> conversion need similar handling of debug insns?
>
> DImode conversion doesn't change register mode (i.e. never calls
> PUT_MODE for registers).  That keeps debug instructions valid.
>
> Overall I don't like the idea of having debug insns in candidates
> set and in chains.  Looks like it is possible to have a chain
> consisting of a debug insn only which is weird (otherwise I don't
> see why we may return false in timode_scalar_chain::convert_insn).

Yes, it can happen:

(insn 11 8 12 2 (parallel [
(set (reg/v:TI 91 [  ])
(plus:TI (reg/v:TI 92 [ a ])
(reg/v:TI 96 [ b ])))
(clobber (reg:CC 17 flags))
]) y.i:5 210 {*addti3_doubleword}
 (expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
(debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [  ])) y.i:5 -1
 (nil))


> What about other possible register uses?  If debug insns are added
> to candidates then NONDEBUG_INSN_P check for uses in
> timode_check_non_convertible_regs becomes invalid, right?

Debug insn has no impact on STV decision.  We just need to convert
register referenced in debug insn from V1TImode to TImode in
timode_scalar_chain::convert_insn.

> If we have (or want) to fix some register uses then it's probably
> would be better to visit register uses when we convert its mode
> and make required fix-ups.  It seems better to me to not involve
> debug insns in analysis phase.

Here is the updated patch to add debug insn, which references the
TImode register which will be converted to V1TImode to queue.
I am testing it now.

> Also I don't think debug insns should be accounted as optimized
> instructions because we would get different number of optimized
> instructions depending on debug info availability which may be
> inconvenient for dump scans (and it is not a real instruction
> optimization).
>
> Thanks,
> Ilya
>
>>
>> Uros.
>>
>>>
>>> H.J.
>>> 
>>> gcc/
>>>
>>> PR target/71549
>>> * config/i386/i386.c (timode_scalar_to_vector_candidate_p):
>>> Return true if debug insn has a variable in TImode register.
>>> (timode_remove_non_convertible_regs): Skip debug insn.
>>> (scalar_chain::convert_insn): Change return type to bool.
>>> (scalar_chain::add_insn): Don't check registers in debug insn.
>>> (dimode_scalar_chain::convert_insn): Change return type to bool
>>> and always return true.
>>> (timode_scalar_chain::convert_insn): Change return type to bool.
>>> Convert V1TImode register to SUBREG TImode in debug insn.  Return
>>> false if debug insn isn't converted.  Otherwise, return true.
>>> (scalar_chain::convert): Increment converted_insns only if
>>> convert_insn returns true.
>>>



-- 
H.J.
From 4229b304b98dff0190922ddd5270a1389321313b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Sun, 19 Jun 2016 12:47:45 -0700
Subject: [PATCH] Convert V1TImode register to TImode in debug insn

TImode register referenced in debug insn can be converted to V1TImode
by scalar to vector optimization.  We need to convert a debug insn if
it references TImode register which will be converted to V1TImode.

gcc/

	PR target/71549
	* config/i386/i386.c (scalar_chain::analyze_register_chain): In
	64-bit mode, add debug insn, which references the register, to
	queue.
	(timode_scalar_chain::convert_insn): Convert V1TImode register
	to SUBREG TImode in debug insn.

gcc/testsuite/

	PR target/71549
	* gcc.target/i386/pr71549.c: New test.
---
 gcc/config/i386/i386.c  | 32 +++-
 gcc/testsuite/gcc.target/i386/pr71549.c | 24 
 2 files changed, 55 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr71549.c

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 56a5b9c..cea632c 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -3229,8 +3229,22 @@ scalar_chain::analyze_register_chain (bitmap candidates, df_ref ref)
   for (chain = DF_REF_CHAIN (ref); chain; chain = chain->next)
 {
   unsigned uid = DF_REF_INSN_UID (chain->ref);
+  rtx_insn *insn = DF_REF_INSN (chain->ref);
 
-  if (!NONDEBUG_INSN_P (DF_REF_INSN (chain->ref)))
+  if (TARGET_64BIT && DEBUG_INSN_P (insn))
+	{
+	  /* In 64-bit mode, if a variable is put in a TImode register,
+	 which may be converted to V1TImode, we need to convert
+	 this 

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Ilya Enkovich]

2016-06-20 16:39 GMT+03:00 Uros Bizjak :
> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
>> TImode register referenced in debug insn can be converted to V1TImode
>> by scalar to vector optimization.  We need to convert a debug insn if
>> it has a variable in a TImode register.
>>
>> Tested on x86-64.  OK for trunk?
>
> Ilya, does this approach look good to you? Also, does DImode STV
> conversion need similar handling of debug insns?

DImode conversion doesn't change register mode (i.e. never calls
PUT_MODE for registers).  That keeps debug instructions valid.

Overall I don't like the idea of having debug insns in candidates
set and in chains.  Looks like it is possible to have a chain
consisting of a debug insn only which is weird (otherwise I don't
see why we may return false in timode_scalar_chain::convert_insn).

What about other possible register uses?  If debug insns are added
to candidates then NONDEBUG_INSN_P check for uses in
timode_check_non_convertible_regs becomes invalid, right?

If we have (or want) to fix some register uses then it's probably
would be better to visit register uses when we convert its mode
and make required fix-ups.  It seems better to me to not involve
debug insns in analysis phase.

Also I don't think debug insns should be accounted as optimized
instructions because we would get different number of optimized
instructions depending on debug info availability which may be
inconvenient for dump scans (and it is not a real instruction
optimization).

Thanks,
Ilya

>
> Uros.
>
>>
>> H.J.
>> 
>> gcc/
>>
>> PR target/71549
>> * config/i386/i386.c (timode_scalar_to_vector_candidate_p):
>> Return true if debug insn has a variable in TImode register.
>> (timode_remove_non_convertible_regs): Skip debug insn.
>> (scalar_chain::convert_insn): Change return type to bool.
>> (scalar_chain::add_insn): Don't check registers in debug insn.
>> (dimode_scalar_chain::convert_insn): Change return type to bool
>> and always return true.
>> (timode_scalar_chain::convert_insn): Change return type to bool.
>> Convert V1TImode register to SUBREG TImode in debug insn.  Return
>> false if debug insn isn't converted.  Otherwise, return true.
>> (scalar_chain::convert): Increment converted_insns only if
>> convert_insn returns true.
>>
>> gcc/testsuite/
>>
>> PR target/71549
>> * gcc.target/i386/pr71549.c: New test.
>> ---
>>  gcc/config/i386/i386.c  | 58 
>> -
>>  gcc/testsuite/gcc.target/i386/pr71549.c | 24 ++
>>  2 files changed, 74 insertions(+), 8 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr71549.c
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index 56a5b9c..e17fc53 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -2845,6 +2845,16 @@ dimode_scalar_to_vector_candidate_p (rtx_insn *insn)
>>  static bool
>>  timode_scalar_to_vector_candidate_p (rtx_insn *insn)
>>  {
>> +  if (DEBUG_INSN_P (insn))
>> +{
>> +  /* If a variable is put in a TImode register, which may be
>> +converted to V1TImode, we need to convert this debug insn.  */
>> +  rtx val = PATTERN (insn);
>> +  return (GET_MODE (val) == TImode
>> + && GET_CODE (val) == VAR_LOCATION
>> + && REG_P (PAT_VAR_LOCATION_LOC (val)));
>> +}
>> +
>>rtx def_set = single_set (insn);
>>
>>if (!def_set)
>> @@ -3012,7 +3022,12 @@ timode_remove_non_convertible_regs (bitmap candidates)
>>
>>EXECUTE_IF_SET_IN_BITMAP (candidates, 0, id, bi)
>>  {
>> -  rtx def_set = single_set (DF_INSN_UID_GET (id)->insn);
>> +  rtx_insn *insn = DF_INSN_UID_GET (id)->insn;
>> +  /* Debug insn isn't a SET insn.  */
>> +  if (DEBUG_INSN_P (insn))
>> +   continue;
>> +
>> +  rtx def_set = single_set (insn);
>>rtx dest = SET_DEST (def_set);
>>rtx src = SET_SRC (def_set);
>>
>> @@ -3111,7 +3126,7 @@ class scalar_chain
>>void add_insn (bitmap candidates, unsigned insn_uid);
>>void analyze_register_chain (bitmap candidates, df_ref ref);
>>virtual void mark_dual_mode_def (df_ref def) = 0;
>> -  virtual void convert_insn (rtx_insn *insn) = 0;
>> +  virtual bool convert_insn (rtx_insn *insn) = 0;
>>virtual void convert_registers () = 0;
>>  };
>>
>> @@ -3123,7 +3138,7 @@ class dimode_scalar_chain : public scalar_chain
>>void mark_dual_mode_def (df_ref def);
>>rtx replace_with_subreg (rtx x, rtx reg, rtx subreg);
>>void replace_with_subreg_in_insn (rtx_insn *insn, rtx reg, rtx subreg);
>> -  void convert_insn (rtx_insn *insn);
>> +  bool convert_insn (rtx_insn *insn);
>>void convert_op (rtx *op, rtx_insn *insn);
>>void convert_reg (unsigned regno);
>>void make_vector_copies (unsigned regno);
>> @@ -3139,7 +3154,7 @@ class timode_scalar_chain : 

Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Uros Bizjak]

On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu  wrote:
> TImode register referenced in debug insn can be converted to V1TImode
> by scalar to vector optimization.  We need to convert a debug insn if
> it has a variable in a TImode register.
>
> Tested on x86-64.  OK for trunk?

Ilya, does this approach look good to you? Also, does DImode STV
conversion need similar handling of debug insns?

Uros.

>
> H.J.
> 
> gcc/
>
> PR target/71549
> * config/i386/i386.c (timode_scalar_to_vector_candidate_p):
> Return true if debug insn has a variable in TImode register.
> (timode_remove_non_convertible_regs): Skip debug insn.
> (scalar_chain::convert_insn): Change return type to bool.
> (scalar_chain::add_insn): Don't check registers in debug insn.
> (dimode_scalar_chain::convert_insn): Change return type to bool
> and always return true.
> (timode_scalar_chain::convert_insn): Change return type to bool.
> Convert V1TImode register to SUBREG TImode in debug insn.  Return
> false if debug insn isn't converted.  Otherwise, return true.
> (scalar_chain::convert): Increment converted_insns only if
> convert_insn returns true.
>
> gcc/testsuite/
>
> PR target/71549
> * gcc.target/i386/pr71549.c: New test.
> ---
>  gcc/config/i386/i386.c  | 58 
> -
>  gcc/testsuite/gcc.target/i386/pr71549.c | 24 ++
>  2 files changed, 74 insertions(+), 8 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/pr71549.c
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 56a5b9c..e17fc53 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -2845,6 +2845,16 @@ dimode_scalar_to_vector_candidate_p (rtx_insn *insn)
>  static bool
>  timode_scalar_to_vector_candidate_p (rtx_insn *insn)
>  {
> +  if (DEBUG_INSN_P (insn))
> +{
> +  /* If a variable is put in a TImode register, which may be
> +converted to V1TImode, we need to convert this debug insn.  */
> +  rtx val = PATTERN (insn);
> +  return (GET_MODE (val) == TImode
> + && GET_CODE (val) == VAR_LOCATION
> + && REG_P (PAT_VAR_LOCATION_LOC (val)));
> +}
> +
>rtx def_set = single_set (insn);
>
>if (!def_set)
> @@ -3012,7 +3022,12 @@ timode_remove_non_convertible_regs (bitmap candidates)
>
>EXECUTE_IF_SET_IN_BITMAP (candidates, 0, id, bi)
>  {
> -  rtx def_set = single_set (DF_INSN_UID_GET (id)->insn);
> +  rtx_insn *insn = DF_INSN_UID_GET (id)->insn;
> +  /* Debug insn isn't a SET insn.  */
> +  if (DEBUG_INSN_P (insn))
> +   continue;
> +
> +  rtx def_set = single_set (insn);
>rtx dest = SET_DEST (def_set);
>rtx src = SET_SRC (def_set);
>
> @@ -3111,7 +3126,7 @@ class scalar_chain
>void add_insn (bitmap candidates, unsigned insn_uid);
>void analyze_register_chain (bitmap candidates, df_ref ref);
>virtual void mark_dual_mode_def (df_ref def) = 0;
> -  virtual void convert_insn (rtx_insn *insn) = 0;
> +  virtual bool convert_insn (rtx_insn *insn) = 0;
>virtual void convert_registers () = 0;
>  };
>
> @@ -3123,7 +3138,7 @@ class dimode_scalar_chain : public scalar_chain
>void mark_dual_mode_def (df_ref def);
>rtx replace_with_subreg (rtx x, rtx reg, rtx subreg);
>void replace_with_subreg_in_insn (rtx_insn *insn, rtx reg, rtx subreg);
> -  void convert_insn (rtx_insn *insn);
> +  bool convert_insn (rtx_insn *insn);
>void convert_op (rtx *op, rtx_insn *insn);
>void convert_reg (unsigned regno);
>void make_vector_copies (unsigned regno);
> @@ -3139,7 +3154,7 @@ class timode_scalar_chain : public scalar_chain
>
>   private:
>void mark_dual_mode_def (df_ref def);
> -  void convert_insn (rtx_insn *insn);
> +  bool convert_insn (rtx_insn *insn);
>/* We don't convert registers to difference size.  */
>void convert_registers () {}
>  };
> @@ -3276,6 +3291,10 @@ scalar_chain::add_insn (bitmap candidates, unsigned 
> int insn_uid)
>bitmap_set_bit (insns, insn_uid);
>
>rtx_insn *insn = DF_INSN_UID_GET (insn_uid)->insn;
> +  /* Debug insn isn't a SET insn.  */
> +  if (DEBUG_INSN_P (insn))
> +return;
> +
>rtx def_set = single_set (insn);
>if (def_set && REG_P (SET_DEST (def_set))
>&& !HARD_REGISTER_P (SET_DEST (def_set)))
> @@ -3708,7 +3727,7 @@ dimode_scalar_chain::convert_op (rtx *op, rtx_insn 
> *insn)
>
>  /* Convert INSN to vector mode.  */
>
> -void
> +bool
>  dimode_scalar_chain::convert_insn (rtx_insn *insn)
>  {
>rtx def_set = single_set (insn);
> @@ -3788,13 +3807,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
>INSN_CODE (insn) = -1;
>recog_memoized (insn);
>df_insn_rescan (insn);
> +
> +  return true;
>  }
>
>  /* Convert INSN from TImode to V1T1mode.  */
>
> -void
> +bool
>  timode_scalar_chain::convert_insn (rtx_insn *insn)
>  {
> +  if