Fix PR rtl-optimization/89588

Message ID 1749039.2VBSkXCst6@polaris
State New
Headers show
Series
  • Fix PR rtl-optimization/89588
Related show

Commit Message

Eric Botcazou March 11, 2019, 9:05 a.m.
Hi,

this is the failure of the assertion:

  /* Should not get here (such loop should be peeled instead).  */
  gcc_assert (niter > max_unroll + 1);

in unroll_loop_constant_iterations on a testcase both containing #pragma GCC 
unroll and compiled with -fno-tree-loop-optimize.  The proposed fix is just to 
disable the pragma altogether when the option is passed.

Tested on x86_64-suse-linux, OK for mainline and 8 branch?


2019-03-11  Eric Botcazou  <ebotcazou@adacore.com>

	PR rtl-optimization/89588
	* tree-cfg.c (replace_loop_annotate_in_block) <annot_expr_unroll_kind>:
	Skip annotation and warn if -fno-tree-loop-optimize is specified.


2019-03-11  Eric Botcazou  <ebotcazou@adacore.com>

	* c-c++-common/unroll-6.c: New test.

-- 
Eric Botcazou

Comments

Richard Biener March 11, 2019, 9:38 a.m. | #1
On Mon, Mar 11, 2019 at 10:05 AM Eric Botcazou <ebotcazou@adacore.com> wrote:
>

> Hi,

>

> this is the failure of the assertion:

>

>   /* Should not get here (such loop should be peeled instead).  */

>   gcc_assert (niter > max_unroll + 1);

>

> in unroll_loop_constant_iterations on a testcase both containing #pragma GCC

> unroll and compiled with -fno-tree-loop-optimize.  The proposed fix is just to

> disable the pragma altogether when the option is passed.

>

> Tested on x86_64-suse-linux, OK for mainline and 8 branch?


Hmm, this looks fragile - isn't the same effect when using
-fdisable-tree-cunroll?

That is, it looks like we could "move" the assert to decide_unrolling
instead, deciding LPT_NONE?

Richard.

>

> 2019-03-11  Eric Botcazou  <ebotcazou@adacore.com>

>

>         PR rtl-optimization/89588

>         * tree-cfg.c (replace_loop_annotate_in_block) <annot_expr_unroll_kind>:

>         Skip annotation and warn if -fno-tree-loop-optimize is specified.

>

>

> 2019-03-11  Eric Botcazou  <ebotcazou@adacore.com>

>

>         * c-c++-common/unroll-6.c: New test.

>

> --

> Eric Botcazou
Eric Botcazou March 11, 2019, 10:14 a.m. | #2
> Hmm, this looks fragile - isn't the same effect when using

> -fdisable-tree-cunroll?


Maybe.

> That is, it looks like we could "move" the assert to decide_unrolling

> instead, deciding LPT_NONE?


We already have a guard for the assertion but it is bypassed here.

I'm going to commit this (equivalent to Jakub's fixlet) after retesting.


	PR rtl-optimization/89588
	* loop-unroll.c (decide_unroll_constant_iterations): Make guard for
	explicit unrolling factor more robust.

-- 
Eric Botcazou
Index: loop-unroll.c
===================================================================
--- loop-unroll.c	(revision 269546)
+++ loop-unroll.c	(working copy)
@@ -400,7 +400,7 @@ decide_unroll_constant_iterations (struc
     {
       /* However we cannot unroll completely at the RTL level a loop with
 	 constant number of iterations; it should have been peeled instead.  */
-      if ((unsigned) loop->unroll - 1 > desc->niter - 2)
+      if (desc->niter < 2 || (unsigned) loop->unroll - 1 > desc->niter - 2)
 	{
 	  if (dump_file)
 	    fprintf (dump_file, ";; Loop should have been peeled\n");

Patch

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 269546)
+++ tree-cfg.c	(working copy)
@@ -280,9 +280,16 @@  replace_loop_annotate_in_block (basic_bl
 	  loop->safelen = INT_MAX;
 	  break;
 	case annot_expr_unroll_kind:
-	  loop->unroll
-	    = (unsigned short) tree_to_shwi (gimple_call_arg (stmt, 2));
-	  cfun->has_unroll = true;
+	  if (flag_tree_loop_optimize)
+	    {
+	      loop->unroll
+		= (unsigned short) tree_to_shwi (gimple_call_arg (stmt, 2));
+	      cfun->has_unroll = true;
+	    }
+	  else
+	    warning_at (gimple_location (stmt), 0,
+			"pragma GCC unroll disabled by "
+			"-fno-tree-loop-optimize");
 	  break;
 	case annot_expr_no_vector_kind:
 	  loop->dont_vectorize = true;