diff mbox

patch for PR69614

Message ID 56E42E36.60509@redhat.com
State New
Headers show

Commit Message

Vladimir Makarov March 12, 2016, 2:56 p.m. UTC 
The following patch should solve the PR which is discussed on

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69614

   The patch was bootstrapped and tested on x86/x86-64.

   Committed as rev. 234162.

Comments

Jakub Jelinek March 12, 2016, 3:19 p.m. UTC | #1 
On Sat, Mar 12, 2016 at 09:56:54AM -0500, Vladimir Makarov wrote:
> +2016-03-12  Vladimir Makarov  <vmakarov@redhat.com>
> +
> +	PR target/69614
> +	* lra-constraints.c (delete_move_and_clobber): New.
> +	(remove_inheritance_pseudos): Use it.

Thanks for working on this.

> --- lra-constraints.c	(revision 234142)
> +++ lra-constraints.c	(working copy)
> @@ -5850,6 +5850,24 @@ get_regno (rtx reg)
>    return -1;
>  }
>  
> +/* Delete a move INSN with destination reg DREGNO and a previous
> +   clobber insn with the same regno.  The inheritance/split code can
> +   generate moves with preceding clobber and when we delete such moves
> +   we should delete the clobber insn too to keep the correct life
> +   info.  */
> +static void
> +delete_move_and_clobber (rtx_insn *insn, int dregno)
> +{
> +  rtx_insn *prev_insn = PREV_INSN (insn);
> +
> +  lra_set_insn_deleted (insn);
> +  lra_assert (dregno > 0);
> +  if (prev_insn != NULL && NONDEBUG_INSN_P (prev_insn)
> +      && GET_CODE (PATTERN (prev_insn)) == CLOBBER
> +      && dregno == get_regno (XEXP (PATTERN (prev_insn), 0)))
> +    lra_set_insn_deleted (prev_insn);
> +}

I just wonder if this isn't at least a latent -fcompare-debug issue.
Using prev_nondebug_insn (insn) instead of PREV_INSN (insn) would look
safer.  If you are sure this is always an insn generated during LRA rather
than preexisting, and there is no chance DEBUG_INSNs could appear in
between, please ignore me.

	Jakub
Jeff Law March 12, 2016, 5:17 p.m. UTC | #2 
On 03/12/2016 07:56 AM, Vladimir Makarov wrote:
>    The following patch should solve the PR which is discussed on
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69614
>
>    The patch was bootstrapped and tested on x86/x86-64.
>
>    Committed as rev. 234162.
>
Isn't this potentially a problem for the gcc-4.9 and gcc-5 release branches?

I'm going to add regression markers for those and drop the gcc-6 
regression marker.

jeff
Christophe Lyon March 14, 2016, 1:40 p.m. UTC | #3 
On 12 March 2016 at 18:17, Jeff Law <law@redhat.com> wrote:
> On 03/12/2016 07:56 AM, Vladimir Makarov wrote:
>>
>>    The following patch should solve the PR which is discussed on
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69614
>>
>>    The patch was bootstrapped and tested on x86/x86-64.
>>
>>    Committed as rev. 234162.
>>

Hi Vladimir,

I've noticed that the newly introduced testcase fails on
armeb-none-linux-gnueabihf,
with GCC configured as:
--with-cpu=cortex-a9 --with-fpu=neon-fp16
whether --with-mode=arm or thumb,
but the test passes if GCC is configured --with-fpu=vfpv3-d16-fp16

I'm using qemu, and the test prints:
000000cef0a2b644
instead of the expected value.

Do you prefer me to update bugzilla with this information?

Thanks,

Christophe.

> Isn't this potentially a problem for the gcc-4.9 and gcc-5 release branches?
>
> I'm going to add regression markers for those and drop the gcc-6 regression
> marker.
>
> jeff
diff mbox

Patch

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 234142)
+++ ChangeLog	(working copy)
@@ -1,3 +1,9 @@ 
+2016-03-12  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR target/69614
+	* lra-constraints.c (delete_move_and_clobber): New.
+	(remove_inheritance_pseudos): Use it.
+
 2016-03-11  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	PR target/70002
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 234142)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,8 @@ 
+2016-03-12  Vladimir Makarov  <vmakarov@redhat.com>
+
+	PR target/69614
+	* gcc.target/arm/pr69614.c: New.
+
 2016-03-11  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
 
 	* gcc.target/aarch64/vect-reduc-or_1.c: Add -fno-vect-cost-model to
Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 234142)
+++ lra-constraints.c	(working copy)
@@ -5850,6 +5850,24 @@  get_regno (rtx reg)
   return -1;
 }
 
+/* Delete a move INSN with destination reg DREGNO and a previous
+   clobber insn with the same regno.  The inheritance/split code can
+   generate moves with preceding clobber and when we delete such moves
+   we should delete the clobber insn too to keep the correct life
+   info.  */
+static void
+delete_move_and_clobber (rtx_insn *insn, int dregno)
+{
+  rtx_insn *prev_insn = PREV_INSN (insn);
+
+  lra_set_insn_deleted (insn);
+  lra_assert (dregno > 0);
+  if (prev_insn != NULL && NONDEBUG_INSN_P (prev_insn)
+      && GET_CODE (PATTERN (prev_insn)) == CLOBBER
+      && dregno == get_regno (XEXP (PATTERN (prev_insn), 0)))
+    lra_set_insn_deleted (prev_insn);
+}
+
 /* Remove inheritance/split pseudos which are in REMOVE_PSEUDOS and
    return true if we did any change.  The undo transformations for
    inheritance looks like
@@ -5922,7 +5940,7 @@  remove_inheritance_pseudos (bitmap remov
 			       ? "split" : "inheritance");
 		      dump_insn_slim (lra_dump_file, curr_insn);
 		    }
-		  lra_set_insn_deleted (curr_insn);
+		  delete_move_and_clobber (curr_insn, dregno);
 		  done_p = true;
 		}
 	      else if (bitmap_bit_p (remove_pseudos, sregno)
@@ -6122,7 +6140,7 @@  undo_optional_reloads (void)
 			       INSN_UID (insn));
 		      dump_insn_slim (lra_dump_file, insn);
 		    }
-		  lra_set_insn_deleted (insn);
+		  delete_move_and_clobber (insn, REGNO (dest));
 		  continue;
 		}
 	      /* We should not worry about generation memory-memory
Index: testsuite/gcc.target/arm/pr69614.c
===================================================================
--- testsuite/gcc.target/arm/pr69614.c	(revision 0)
+++ testsuite/gcc.target/arm/pr69614.c	(working copy)
@@ -0,0 +1,39 @@ 
+/* { dg-do run } */
+/* { dg-require-effective-target arm_arch_v7a_ok } */
+/* { dg-options "-Os -w -fno-expensive-optimizations -fschedule-insns -mtpcs-leaf-frame -fira-algorithm=priority" } */
+
+
+typedef unsigned short u16;
+typedef unsigned short v16u16 __attribute__ ((vector_size (16)));
+typedef unsigned int u32;
+typedef unsigned int v16u32 __attribute__ ((vector_size (16)));
+typedef unsigned long long u64;
+typedef unsigned long long v16u64 __attribute__ ((vector_size (16)));
+
+u64 __attribute__ ((noinline, noclone))
+foo(u16 u16_0, u32 u32_0, u64 u64_0, u16 u16_1, u32 u32_1, u64 u64_1,
+    v16u16 v16u16_0, v16u32 v16u32_0, v16u64 v16u64_0, v16u16 v16u16_1, v16u32 v16u32_1, v16u64 v16u64_1)
+{
+  v16u64_0 %= (v16u64){(u16) v16u16_0[5], ~v16u64_1[1]};
+  v16u64_0[1] = 1;
+  v16u32_1[3] >>= 31;
+  v16u64_1 ^= (v16u64){v16u16_1[4], u64_1};
+  v16u64_1[0] = (v16u64_1[0] >> 63) | (v16u64_1[0] << 1);
+  u16_0 -= 1;
+  v16u32_1 %= (v16u32)-v16u64_0 | 1;
+  v16u16_0 /= (v16u16){-u64_1} | 1;
+  v16u32_0[2] |= (u16)~u16_1;
+    return u16_0 + u64_0 + u32_1 + u64_1 +
+                v16u16_0[0] + v16u16_0[1] + v16u16_0[2] + v16u16_0[3] + v16u16_0[4] + v16u16_0[5] + v16u16_0[6] + v16u32_0[2] + v16u32_0[3] + v16u64_0[0] +
+      v16u16_1[2] + v16u16_1[4] + v16u32_1[0] + v16u32_1[1] + v16u32_1[2] + v16u32_1[3] + v16u64_1[0] + v16u64_1[1];
+}
+
+int
+main ()
+{
+  u64 x = foo(0, 0, 1, 0, 0, 1, (v16u16){-1, 0, 0, 0, 0, 1}, (v16u32){0}, (v16u64){0}, (v16u16){0}, (v16u32){0}, (v16u64){0x67784fdb22, 1});
+  __builtin_printf ("%016llx\n", (unsigned long long) (x >> 0));
+  if (x != 0x000000cef0a1b646)
+    __builtin_abort();
+  return 0;
+}