diff mbox series

[GRAPHITE] Fix PR71351

Message ID alpine.LSU.2.20.1709211204180.26836@zhemvz.fhfr.qr
State New
Headers show
Series [GRAPHITE] Fix PR71351 | expand

Commit Message

Richard Biener Sept. 21, 2017, 10:07 a.m. UTC
This PR is about code generation issues with us inserting "loop header
copies" in the attempt to cover up cases where the loop doesn't run.
But we are disregarding such cases early already.  Thus the simple
fix is to not emit those - we have no idea what values to use
for reduction results anyway.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

2017-09-21  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/71351
	* graphite-isl-ast-to-gimple.c (translate_isl_ast_to_gimple::
	graphite_create_new_loop_guard): Remove, fold remaining parts
	into caller ...
	(translate_isl_ast_node_for): ... here and simplify.

	* gfortran.dg/graphite/pr71351.f90: New testcase.
	* gfortran.dg/graphite/interchange-3.f90: Adjust.

Comments

Sebastian Pop Sept. 21, 2017, 1:43 p.m. UTC | #1
On Thu, Sep 21, 2017 at 5:07 AM, Richard Biener <rguenther@suse.de> wrote:

>
> This PR is about code generation issues with us inserting "loop header
> copies" in the attempt to cover up cases where the loop doesn't run.
> But we are disregarding such cases early already.  Thus the simple
> fix is to not emit those - we have no idea what values to use
> for reduction results anyway.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
>
> 2017-09-21  Richard Biener  <rguenther@suse.de>
>
>         PR tree-optimization/71351
>         * graphite-isl-ast-to-gimple.c (translate_isl_ast_to_gimple::
>         graphite_create_new_loop_guard): Remove, fold remaining parts
>         into caller ...
>         (translate_isl_ast_node_for): ... here and simplify.
>
>         * gfortran.dg/graphite/pr71351.f90: New testcase.
>         * gfortran.dg/graphite/interchange-3.f90: Adjust.
>
>
Looks good to me.

Thanks,
Sebastian
Tom de Vries Dec. 19, 2017, 12:38 p.m. UTC | #2
On 09/21/2017 12:07 PM, Richard Biener wrote:
> -    exit_edge = create_empty_if_region_on_edge (entry_edge,
> -						unshare_expr (cond_expr));

This removes the fix for PR70045:
...
diff --git a/gcc/graphite-isl-ast-to-gimple.c 
b/gcc/graphite-isl-ast-to-gimple.c
index 89a4118..8dd5dc8 100644
--- a/gcc/graphite-isl-ast-to-gimple.c
+++ b/gcc/graphite-isl-ast-to-gimple.c
@@ -821,7 +821,8 @@ graphite_create_new_loop_guard (edge entry_edge,
    if (integer_onep (cond_expr))
      exit_edge = entry_edge;
    else
-    exit_edge = create_empty_if_region_on_edge (entry_edge, cond_expr);
+    exit_edge = create_empty_if_region_on_edge (entry_edge,
+						unshare_expr (cond_expr));

    return exit_edge;
  }
...


Consequently, the pr70045.c testcase is currently ICE-ing.

Attached patch fixes this.

OK for trunk if bootstrap and reg-test on x86_64 succeed?

Thanks,
- Tom
diff --git a/gcc/graphite-isl-ast-to-gimple.c b/gcc/graphite-isl-ast-to-gimple.c
index 848bfe9..b020b2d 100644
--- a/gcc/graphite-isl-ast-to-gimple.c
+++ b/gcc/graphite-isl-ast-to-gimple.c
@@ -739,10 +739,10 @@ translate_isl_ast_node_for (loop_p context_loop, __isl_keep isl_ast_node *node,
 	 as expected.  */
       tree ub_one = fold_build2 (POINTER_TYPE_P (type)
 				 ? POINTER_PLUS_EXPR : PLUS_EXPR,
-				 type, ub, one);
+				 type, unshare_expr (ub), one);
       create_empty_if_region_on_edge (next_e,
 				      fold_build2 (LT_EXPR, boolean_type_node,
-						   lb, ub_one));
+						   unshare_expr (lb), ub_one));
       next_e = get_true_edge_from_guard_bb (next_e->dest);
     }
Richard Biener Dec. 19, 2017, 1:05 p.m. UTC | #3
On Tue, 19 Dec 2017, Tom de Vries wrote:

> On 09/21/2017 12:07 PM, Richard Biener wrote:
> > -    exit_edge = create_empty_if_region_on_edge (entry_edge,
> > -						unshare_expr (cond_expr));
> 
> This removes the fix for PR70045:
> ...
> diff --git a/gcc/graphite-isl-ast-to-gimple.c
> b/gcc/graphite-isl-ast-to-gimple.c
> index 89a4118..8dd5dc8 100644
> --- a/gcc/graphite-isl-ast-to-gimple.c
> +++ b/gcc/graphite-isl-ast-to-gimple.c
> @@ -821,7 +821,8 @@ graphite_create_new_loop_guard (edge entry_edge,
>    if (integer_onep (cond_expr))
>      exit_edge = entry_edge;
>    else
> -    exit_edge = create_empty_if_region_on_edge (entry_edge, cond_expr);
> +    exit_edge = create_empty_if_region_on_edge (entry_edge,
> +						unshare_expr (cond_expr));
> 
>    return exit_edge;
>  }
> ...
> 
> 
> Consequently, the pr70045.c testcase is currently ICE-ing.

Sorry.

> Attached patch fixes this.
> 
> OK for trunk if bootstrap and reg-test on x86_64 succeed?

Ok.

Richard.
Tom de Vries Dec. 19, 2017, 1:44 p.m. UTC | #4
On 12/19/2017 02:05 PM, Richard Biener wrote:
> On Tue, 19 Dec 2017, Tom de Vries wrote:
> 
>> On 09/21/2017 12:07 PM, Richard Biener wrote:
>>> -    exit_edge = create_empty_if_region_on_edge (entry_edge,
>>> -						unshare_expr (cond_expr));
>>
>> This removes the fix for PR70045:
>> ...
>> diff --git a/gcc/graphite-isl-ast-to-gimple.c
>> b/gcc/graphite-isl-ast-to-gimple.c
>> index 89a4118..8dd5dc8 100644
>> --- a/gcc/graphite-isl-ast-to-gimple.c
>> +++ b/gcc/graphite-isl-ast-to-gimple.c
>> @@ -821,7 +821,8 @@ graphite_create_new_loop_guard (edge entry_edge,
>>     if (integer_onep (cond_expr))
>>       exit_edge = entry_edge;
>>     else
>> -    exit_edge = create_empty_if_region_on_edge (entry_edge, cond_expr);
>> +    exit_edge = create_empty_if_region_on_edge (entry_edge,
>> +						unshare_expr (cond_expr));
>>
>>     return exit_edge;
>>   }
>> ...
>>
>>
>> Consequently, the pr70045.c testcase is currently ICE-ing.
> 
> Sorry.
> 

Np. Nice to see that the regression test caught it :)

>> Attached patch fixes this.
>>
>> OK for trunk if bootstrap and reg-test on x86_64 succeed?
> 
> Ok.

Thanks,
- Tom
diff mbox series

Patch

Index: gcc/graphite-isl-ast-to-gimple.c
===================================================================
--- gcc/graphite-isl-ast-to-gimple.c	(revision 253008)
+++ gcc/graphite-isl-ast-to-gimple.c	(working copy)
@@ -193,10 +193,6 @@  class translate_isl_ast_to_gimple
 					 __isl_keep isl_ast_node *node_for,
 					 loop_p outer, tree type,
 					 tree lb, tree ub, ivs_params &ip);
-  edge graphite_create_new_loop_guard (edge entry_edge,
-				       __isl_keep isl_ast_node *node_for,
-				       tree *type,
-				       tree *lb, tree *ub, ivs_params &ip);
   edge graphite_create_new_guard (edge entry_edge,
 				  __isl_take isl_ast_expr *if_cond,
 				  ivs_params &ip);
@@ -731,71 +727,6 @@  get_upper_bound (__isl_keep isl_ast_node
   return res;
 }
 
-/* All loops generated by create_empty_loop_on_edge have the form of
-   a post-test loop:
-
-   do
-
-   {
-     body of the loop;
-   } while (lower bound < upper bound);
-
-   We create a new if region protecting the loop to be executed, if
-   the execution count is zero (lower bound > upper bound).  */
-
-edge translate_isl_ast_to_gimple::
-graphite_create_new_loop_guard (edge entry_edge,
-				__isl_keep isl_ast_node *node_for, tree *type,
-				tree *lb, tree *ub, ivs_params &ip)
-{
-  gcc_assert (isl_ast_node_get_type (node_for) == isl_ast_node_for);
-  tree cond_expr;
-  edge exit_edge;
-
-  *type =
-    build_nonstandard_integer_type (graphite_expression_type_precision, 0);
-  isl_ast_expr *for_init = isl_ast_node_for_get_init (node_for);
-  *lb = gcc_expression_from_isl_expression (*type, for_init, ip);
-
-  /* To fail code generation, we generate wrong code until we discard it.  */
-  if (codegen_error_p ())
-    *lb = integer_zero_node;
-
-  isl_ast_expr *upper_bound = get_upper_bound (node_for);
-  *ub = gcc_expression_from_isl_expression (*type, upper_bound, ip);
-
-  /* To fail code generation, we generate wrong code until we discard it.  */
-  if (codegen_error_p ())
-    *ub = integer_zero_node;
-  
-  /* When ub is simply a constant or a parameter, use lb <= ub.  */
-  if (TREE_CODE (*ub) == INTEGER_CST || TREE_CODE (*ub) == SSA_NAME)
-    cond_expr = fold_build2 (LE_EXPR, boolean_type_node, *lb, *ub);
-  else
-    {
-      tree one = (POINTER_TYPE_P (*type)
-		  ? convert_to_ptrofftype (integer_one_node)
-		  : fold_convert (*type, integer_one_node));
-      /* Adding +1 and using LT_EXPR helps with loop latches that have a
-	 loop iteration count of "PARAMETER - 1".  For PARAMETER == 0 this
-	 becomes 2^k-1 due to integer overflow, and the condition lb <= ub
-	 is true, even if we do not want this.  However lb < ub + 1 is false,
-	 as expected.  */
-      tree ub_one = fold_build2 (POINTER_TYPE_P (*type) ? POINTER_PLUS_EXPR
-				 : PLUS_EXPR, *type, *ub, one);
-
-      cond_expr = fold_build2 (LT_EXPR, boolean_type_node, *lb, ub_one);
-    }
-
-  if (integer_onep (cond_expr))
-    exit_edge = entry_edge;
-  else
-    exit_edge = create_empty_if_region_on_edge (entry_edge,
-						unshare_expr (cond_expr));
-
-  return exit_edge;
-}
-
 /* Translates an isl_ast_node_for to Gimple. */
 
 edge translate_isl_ast_to_gimple::
@@ -803,26 +734,24 @@  translate_isl_ast_node_for (loop_p conte
 			    edge next_e, ivs_params &ip)
 {
   gcc_assert (isl_ast_node_get_type (node) == isl_ast_node_for);
-  tree type, lb, ub;
-  edge last_e = graphite_create_new_loop_guard (next_e, node, &type,
-						&lb, &ub, ip);
+  tree type
+    = build_nonstandard_integer_type (graphite_expression_type_precision, 0);
 
-  if (last_e == next_e)
-    {
-      /* There was no guard generated.  */
-      last_e = single_succ_edge (split_edge (last_e));
-
-      translate_isl_ast_for_loop (context_loop, node, next_e,
-				  type, lb, ub, ip);
-      return last_e;
-    }
-
-  edge true_e = get_true_edge_from_guard_bb (next_e->dest);
-  merge_points.safe_push (last_e);
+  isl_ast_expr *for_init = isl_ast_node_for_get_init (node);
+  tree lb = gcc_expression_from_isl_expression (type, for_init, ip);
+  /* To fail code generation, we generate wrong code until we discard it.  */
+  if (codegen_error_p ())
+    lb = integer_zero_node;
 
-  last_e = single_succ_edge (split_edge (last_e));
-  translate_isl_ast_for_loop (context_loop, node, true_e, type, lb, ub, ip);
+  isl_ast_expr *upper_bound = get_upper_bound (node);
+  tree ub = gcc_expression_from_isl_expression (type, upper_bound, ip);
+  /* To fail code generation, we generate wrong code until we discard it.  */
+  if (codegen_error_p ())
+    ub = integer_zero_node;
 
+  edge last_e = single_succ_edge (split_edge (next_e));
+  translate_isl_ast_for_loop (context_loop, node, next_e,
+			      type, lb, ub, ip);
   return last_e;
 }
 
Index: gcc/testsuite/gfortran.dg/graphite/pr71351.f90
===================================================================
--- gcc/testsuite/gfortran.dg/graphite/pr71351.f90	(nonexistent)
+++ gcc/testsuite/gfortran.dg/graphite/pr71351.f90	(working copy)
@@ -0,0 +1,15 @@ 
+! { dg-do compile }
+! { dg-options "-O2 -floop-nest-optimize" }
+
+SUBROUTINE print_crys_symmetry(nc,v)
+  INTEGER :: nc
+  REAL(KIND=8), DIMENSION(3,48) :: v
+  INTEGER  :: n,i
+  vs = 0.0_8
+  DO n = 1, nc 
+     DO i = 1, 3
+        vs = vs + ABS(v(i,n))
+     END DO
+  END DO
+  CALL foo(vs)
+END SUBROUTINE print_crys_symmetry
Index: gcc/testsuite/gfortran.dg/graphite/interchange-3.f90
===================================================================
--- gcc/testsuite/gfortran.dg/graphite/interchange-3.f90	(revision 253008)
+++ gcc/testsuite/gfortran.dg/graphite/interchange-3.f90	(working copy)
@@ -24,4 +24,4 @@  Program FOO
 
 end Program FOO
 
-! { dg-final { scan-tree-dump-times "codegen error: reverting back to the original code." "1" "graphite" } }
+! { dg-final { scan-tree-dump "tiled" "graphite" } }