diff mbox

Fix x86_64 fix_debug_reg_uses (PR rtl-optimization/78575)

Message ID 20161129194158.GI3541@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Nov. 29, 2016, 7:41 p.m. UTC 
Hi!

The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
all setters and users, but that is undesirable in debug insns which are
intentionally ignored during the analysis and we should keep using correct
modes (TImode) instead of the new one (V1TImode).

The current fix_debug_reg_uses implementation just assumes such a pseudo
can appear only directly in the VAR_LOCATION's second operand, but it can of
course appear anywhere in the expression, the whole expression doesn't have
to be TImode either (e.g. on the testcase it is a QImode comparison of
originally TImode pseudo with CONST_INT, which stv incorrectly changes into
comparison of V1TImode with CONST_INT).

The following patch fixes that and also fixes an issue if the pseudo appears
multiple times in the debug info that the rescan could break traversal of
further uses.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-11-29  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/78575
	* config/i386/i386.c (timode_scalar_chain::fix_debug_reg_uses): Use
	DF infrastructure to wrap all V1TImode reg uses into TImode subreg
	if not already wrapped in a subreg.  Make sure df_insn_rescan does not
	affect further iterations.

	* gcc.dg/pr78575.c: New test.


	Jakub

Comments

Jeff Law Nov. 29, 2016, 10:20 p.m. UTC | #1 
On 11/29/2016 12:41 PM, Jakub Jelinek wrote:
> Hi!
>
> The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
> all setters and users, but that is undesirable in debug insns which are
> intentionally ignored during the analysis and we should keep using correct
> modes (TImode) instead of the new one (V1TImode).
Note that MEMs are not shared, so twiddling the mode on any given MEM 
changes one and only one object.

Jeff
Jakub Jelinek Nov. 29, 2016, 10:26 p.m. UTC | #2 
On Tue, Nov 29, 2016 at 03:20:08PM -0700, Jeff Law wrote:
> On 11/29/2016 12:41 PM, Jakub Jelinek wrote:
> >The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
> >all setters and users, but that is undesirable in debug insns which are
> >intentionally ignored during the analysis and we should keep using correct
> >modes (TImode) instead of the new one (V1TImode).
> Note that MEMs are not shared, so twiddling the mode on any given MEM
> changes one and only one object.

I thought they shouldn't be shared.  Which means I'll debug tomorrow why
they are shared (the DECL_INCOMING_RTL is shared with a REG_EQUAL note
content).

	Jakub
Jakub Jelinek Nov. 30, 2016, 6:57 p.m. UTC | #3 
On Tue, Nov 29, 2016 at 03:20:08PM -0700, Jeff Law wrote:
> On 11/29/2016 12:41 PM, Jakub Jelinek wrote:
> >Hi!
> >
> >The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
> >all setters and users, but that is undesirable in debug insns which are
> >intentionally ignored during the analysis and we should keep using correct
> >modes (TImode) instead of the new one (V1TImode).
> Note that MEMs are not shared, so twiddling the mode on any given MEM
> changes one and only one object.

Note that this patch isn't trying to workaround any wrong MEM sharing,
while the other patch has been.  So, is the PR78575 ok as is?
I'll post the other updated patch in the corresponding thread.

	Jakub
Jeff Law Dec. 1, 2016, 10:55 p.m. UTC | #4 
On 11/30/2016 11:57 AM, Jakub Jelinek wrote:
> On Tue, Nov 29, 2016 at 03:20:08PM -0700, Jeff Law wrote:
>> On 11/29/2016 12:41 PM, Jakub Jelinek wrote:
>>> Hi!
>>>
>>> The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
>>> all setters and users, but that is undesirable in debug insns which are
>>> intentionally ignored during the analysis and we should keep using correct
>>> modes (TImode) instead of the new one (V1TImode).
>> Note that MEMs are not shared, so twiddling the mode on any given MEM
>> changes one and only one object.
>
> Note that this patch isn't trying to workaround any wrong MEM sharing,
> while the other patch has been.  So, is the PR78575 ok as is?
> I'll post the other updated patch in the corresponding thread.
It was an x86 patch, right?  So it's Uros's call.

jeff
Jakub Jelinek Dec. 1, 2016, 11:14 p.m. UTC | #5 
On Thu, Dec 01, 2016 at 03:55:58PM -0700, Jeff Law wrote:
> On 11/30/2016 11:57 AM, Jakub Jelinek wrote:
> >On Tue, Nov 29, 2016 at 03:20:08PM -0700, Jeff Law wrote:
> >>On 11/29/2016 12:41 PM, Jakub Jelinek wrote:
> >>>Hi!
> >>>
> >>>The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
> >>>all setters and users, but that is undesirable in debug insns which are
> >>>intentionally ignored during the analysis and we should keep using correct
> >>>modes (TImode) instead of the new one (V1TImode).
> >>Note that MEMs are not shared, so twiddling the mode on any given MEM
> >>changes one and only one object.
> >
> >Note that this patch isn't trying to workaround any wrong MEM sharing,
> >while the other patch has been.  So, is the PR78575 ok as is?
> >I'll post the other updated patch in the corresponding thread.
> It was an x86 patch, right?  So it's Uros's call.

Yeah, it is config/i386/ only.

	Jakub
Uros Bizjak Dec. 2, 2016, 7:38 a.m. UTC | #6 
On Tue, Nov 29, 2016 at 8:41 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The x86_64 stv pass uses PUT_MODE to change REGs and MEMs in place to affect
> all setters and users, but that is undesirable in debug insns which are
> intentionally ignored during the analysis and we should keep using correct
> modes (TImode) instead of the new one (V1TImode).
>
> The current fix_debug_reg_uses implementation just assumes such a pseudo
> can appear only directly in the VAR_LOCATION's second operand, but it can of
> course appear anywhere in the expression, the whole expression doesn't have
> to be TImode either (e.g. on the testcase it is a QImode comparison of
> originally TImode pseudo with CONST_INT, which stv incorrectly changes into
> comparison of V1TImode with CONST_INT).
>
> The following patch fixes that and also fixes an issue if the pseudo appears
> multiple times in the debug info that the rescan could break traversal of
> further uses.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-11-29  Jakub Jelinek  <jakub@redhat.com>
>
>         PR rtl-optimization/78575
>         * config/i386/i386.c (timode_scalar_chain::fix_debug_reg_uses): Use
>         DF infrastructure to wrap all V1TImode reg uses into TImode subreg
>         if not already wrapped in a subreg.  Make sure df_insn_rescan does not
>         affect further iterations.
>
>         * gcc.dg/pr78575.c: New test.

OK.

Thanks,
Uros.

> --- gcc/config/i386/i386.c.jj   2016-11-28 10:59:08.000000000 +0100
> +++ gcc/config/i386/i386.c      2016-11-29 08:31:58.061278522 +0100
> @@ -3831,30 +3831,32 @@ timode_scalar_chain::fix_debug_reg_uses
>    if (!flag_var_tracking)
>      return;
>
> -  df_ref ref;
> -  for (ref = DF_REG_USE_CHAIN (REGNO (reg));
> -       ref;
> -       ref = DF_REF_NEXT_REG (ref))
> +  df_ref ref, next;
> +  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = next)
>      {
>        rtx_insn *insn = DF_REF_INSN (ref);
> +      /* Make sure the next ref is for a different instruction,
> +         so that we're not affected by the rescan.  */
> +      next = DF_REF_NEXT_REG (ref);
> +      while (next && DF_REF_INSN (next) == insn)
> +       next = DF_REF_NEXT_REG (next);
> +
>        if (DEBUG_INSN_P (insn))
>         {
>           /* It may be a debug insn with a TImode variable in
>              register.  */
> -         rtx val = PATTERN (insn);
> -         if (GET_MODE (val) != TImode)
> -           continue;
> -         gcc_assert (GET_CODE (val) == VAR_LOCATION);
> -         rtx loc = PAT_VAR_LOCATION_LOC (val);
> -         /* It may have been converted to TImode already.  */
> -         if (GET_MODE (loc) == TImode)
> -           continue;
> -         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);
> +         bool changed = false;
> +         for (; ref != next; ref = DF_REF_NEXT_REG (ref))
> +           {
> +             rtx *loc = DF_REF_LOC (ref);
> +             if (REG_P (*loc) && GET_MODE (*loc) == V1TImode)
> +               {
> +                 *loc = gen_rtx_SUBREG (TImode, *loc, 0);
> +                 changed = true;
> +               }
> +           }
> +         if (changed)
> +           df_insn_rescan (insn);
>         }
>      }
>  }
> --- gcc/testsuite/gcc.dg/pr78575.c.jj   2016-11-29 08:36:25.821932436 +0100
> +++ gcc/testsuite/gcc.dg/pr78575.c      2016-11-29 08:35:35.000000000 +0100
> @@ -0,0 +1,16 @@
> +/* PR rtl-optimization/78575 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2 -g -Wno-psabi" } */
> +
> +typedef unsigned __int128 V __attribute__((vector_size(64)));
> +
> +V g;
> +
> +void
> +foo (V v)
> +{
> +  unsigned __int128 x = 1;
> +  int c = v[1] <= ~x;
> +  v &= v[1];
> +  g = v;
> +}
>
>         Jakub
diff mbox

Patch

--- gcc/config/i386/i386.c.jj	2016-11-28 10:59:08.000000000 +0100
+++ gcc/config/i386/i386.c	2016-11-29 08:31:58.061278522 +0100
@@ -3831,30 +3831,32 @@  timode_scalar_chain::fix_debug_reg_uses
   if (!flag_var_tracking)
     return;
 
-  df_ref ref;
-  for (ref = DF_REG_USE_CHAIN (REGNO (reg));
-       ref;
-       ref = DF_REF_NEXT_REG (ref))
+  df_ref ref, next;
+  for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = next)
     {
       rtx_insn *insn = DF_REF_INSN (ref);
+      /* Make sure the next ref is for a different instruction,
+         so that we're not affected by the rescan.  */
+      next = DF_REF_NEXT_REG (ref);
+      while (next && DF_REF_INSN (next) == insn)
+	next = DF_REF_NEXT_REG (next);
+
       if (DEBUG_INSN_P (insn))
 	{
 	  /* It may be a debug insn with a TImode variable in
 	     register.  */
-	  rtx val = PATTERN (insn);
-	  if (GET_MODE (val) != TImode)
-	    continue;
-	  gcc_assert (GET_CODE (val) == VAR_LOCATION);
-	  rtx loc = PAT_VAR_LOCATION_LOC (val);
-	  /* It may have been converted to TImode already.  */
-	  if (GET_MODE (loc) == TImode)
-	    continue;
-	  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);
+	  bool changed = false;
+	  for (; ref != next; ref = DF_REF_NEXT_REG (ref))
+	    {
+	      rtx *loc = DF_REF_LOC (ref);
+	      if (REG_P (*loc) && GET_MODE (*loc) == V1TImode)
+		{
+		  *loc = gen_rtx_SUBREG (TImode, *loc, 0);
+		  changed = true;
+		}
+	    }
+	  if (changed)
+	    df_insn_rescan (insn);
 	}
     }
 }
--- gcc/testsuite/gcc.dg/pr78575.c.jj	2016-11-29 08:36:25.821932436 +0100
+++ gcc/testsuite/gcc.dg/pr78575.c	2016-11-29 08:35:35.000000000 +0100
@@ -0,0 +1,16 @@ 
+/* PR rtl-optimization/78575 */
+/* { dg-do compile { target int128 } } */
+/* { dg-options "-O2 -g -Wno-psabi" } */
+
+typedef unsigned __int128 V __attribute__((vector_size(64)));
+
+V g;
+
+void
+foo (V v)
+{
+  unsigned __int128 x = 1;
+  int c = v[1] <= ~x;
+  v &= v[1];
+  g = v;
+}