diff mbox

PR 66076: invalid vec_grow in rtx iterators

Message ID 87y4kvtri6.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford May 11, 2015, 9:04 a.m. UTC 
The rtx iterators start out using a stack worklist but fall back to
a heap worklist if the stack one turns out to be too small (which is rare).
PR 66076 showed up a bug in the code that makes the switch from the stack
worklist to the heap worklist: the heap one might already have been
allocated if the worklist structure was shared with a previous
FOR_EACH_SUBRTX and if that FOR_EACH_SUBRTX also blew the stack worklist.
Fixed by making the vec_safe_grow conditional on a vec_safe_length check.
(Note that vec_safe_grow starts with a call to vec_safe_length,
so this is no less efficient than a check for a null pointer.)

If one FOR_EACH_SUBRTX needs to use the heap worklist, it might be slightly
more efficient for future FOR_EACH_SUBRTXes on the same worklist structure
to start out using the heap worklist, in order to avoid any future stack-to-heap
memcpy.  The problem is that that would require extra set-up code for all
FOR_EACH_SUBRTXes, and the idea is to optimise for the common case where
the stack worklist is enough.  The situation handled by the patch should
be very rare -- backed up by the fact that the ICE would always trigger
in that case, yet it didn't show up in pre-release testing.

Maybe it would be worth bumping up the size of the stack worklist to cope
with those long AVX512 constants though.

Tested on x86_64-linux-gnu.  Also tested that the testcase passes on
aarch64-elf.  OK to install?

Thanks,
Richard


gcc/
	PR rtl-optimization/66076
	* rtlanal.c (generic_subrtx_iterator <T>::add_single_to_queue):
	Don't grow the heap array if it is already big enough from a
	previous iteration.

gcc/testsuite/
	PR rtl-optimization/66076
	* gcc.dg/torture/pr66076.c: New test.

Comments

Eric Botcazou May 11, 2015, 9:14 a.m. UTC | #1 
> gcc/
> 	PR rtl-optimization/66076
> 	* rtlanal.c (generic_subrtx_iterator <T>::add_single_to_queue):
> 	Don't grow the heap array if it is already big enough from a
> 	previous iteration.
> 
> gcc/testsuite/
> 	PR rtl-optimization/66076
> 	* gcc.dg/torture/pr66076.c: New test.

OK, thanks.
diff mbox

Patch

Index: gcc/rtlanal.c
===================================================================
--- gcc/rtlanal.c	2015-05-10 10:44:24.091059530 +0100
+++ gcc/rtlanal.c	2015-05-10 20:49:56.618230132 +0100
@@ -104,7 +104,10 @@  generic_subrtx_iterator <T>::add_single_
 	  return base;
 	}
       gcc_checking_assert (i == LOCAL_ELEMS);
-      vec_safe_grow (array.heap, i + 1);
+      /* A previous iteration might also have moved from the stack to the
+	 heap, in which case the heap array will already be big enough.  */
+      if (vec_safe_length (array.heap) <= i)
+	vec_safe_grow (array.heap, i + 1);
       base = array.heap->address ();
       memcpy (base, array.stack, sizeof (array.stack));
       base[LOCAL_ELEMS] = x;
Index: gcc/testsuite/gcc.dg/torture/pr66076.c
===================================================================
--- /dev/null	2015-04-20 08:05:53.260830006 +0100
+++ gcc/testsuite/gcc.dg/torture/pr66076.c	2015-05-10 20:49:56.626230040 +0100
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-options "" } */
+/* { dg-options "-mno-prefer-avx128 -march=bdver4" { target i?86-*-* x86_64-*-* } } */
+
+void
+f0a (char *result, char *arg1, char *arg4, char temp_6)
+{
+  int idx = 0;
+  for (idx = 0; idx < 416; idx += 1)
+    result[idx] = (arg1[idx] + arg4[idx]) * temp_6;
+}