diff mbox series

Non-INTEGER_CST CHREC_RIGHTs in analyze_*_subscript

Message ID 87h8u7w4hj.fsf@linaro.org
State New
Headers show
Series Non-INTEGER_CST CHREC_RIGHTs in analyze_*_subscript | expand

Commit Message

Richard Sandiford Nov. 6, 2017, 8:12 p.m. UTC
initialize_matrix_A requires the CHREC_RIGHT to be an INTEGER_CST:

  switch (TREE_CODE (chrec))
    {
    case POLYNOMIAL_CHREC:
      A[index][0] = mult * int_cst_value (CHREC_RIGHT (chrec));
      return initialize_matrix_A (A, CHREC_LEFT (chrec), index + 1, mult);

and isn't able to back out if it isn't.  This patch instead
checks for an appropriate CHREC_RIGHT before calling the parent
function analyze_subscript_affine_affine.

Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
OK to install?

Richard


2017-11-06  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	* tree-data-ref.c (analyze_siv_subscript): Only call
	analyze_subscript_affine_affine on chrecs that have an
	INTEGER_CST step.
	(analyze_miv_subscript): Likewise.

Comments

Richard Biener Nov. 7, 2017, 9:59 a.m. UTC | #1
On Mon, Nov 6, 2017 at 9:12 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> initialize_matrix_A requires the CHREC_RIGHT to be an INTEGER_CST:

>

>   switch (TREE_CODE (chrec))

>     {

>     case POLYNOMIAL_CHREC:

>       A[index][0] = mult * int_cst_value (CHREC_RIGHT (chrec));

>       return initialize_matrix_A (A, CHREC_LEFT (chrec), index + 1, mult);

>

> and isn't able to back out if it isn't.  This patch instead

> checks for an appropriate CHREC_RIGHT before calling the parent

> function analyze_subscript_affine_affine.

>

> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.

> OK to install?


Hmm.  I think this papers over some underlying issues.  The calls are
guarded already with predicates that are supposed to veryify what you do.
Namely chrec_contains_symbols (a poly_int _does_ contain a "symbol").

So can you please fix that by adding TREE_CODE (chrec) == POLY_INT
to the return true case?

Thanks,
Richard.

> Richard

>

>

> 2017-11-06  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

>         * tree-data-ref.c (analyze_siv_subscript): Only call

>         analyze_subscript_affine_affine on chrecs that have an

>         INTEGER_CST step.

>         (analyze_miv_subscript): Likewise.

>

> Index: gcc/tree-data-ref.c

> ===================================================================

> --- gcc/tree-data-ref.c 2017-11-06 19:39:32.924734707 +0000

> +++ gcc/tree-data-ref.c 2017-11-06 20:10:40.704478657 +0000

> @@ -3788,7 +3788,9 @@ analyze_siv_subscript (tree chrec_a,

>                                       overlaps_b, overlaps_a, last_conflicts);

>

>    else if (evolution_function_is_affine_in_loop (chrec_a, loop_nest_num)

> -          && evolution_function_is_affine_in_loop (chrec_b, loop_nest_num))

> +          && evolution_function_right_is_integer_cst (chrec_a)

> +          && evolution_function_is_affine_in_loop (chrec_b, loop_nest_num)

> +          && evolution_function_right_is_integer_cst (chrec_b))

>      {

>        if (!chrec_contains_symbols (chrec_a)

>           && !chrec_contains_symbols (chrec_b))

> @@ -3922,8 +3924,10 @@ analyze_miv_subscript (tree chrec_a,

>

>    else if (evolution_function_is_affine_multivariate_p (chrec_a, loop_nest->num)

>            && !chrec_contains_symbols (chrec_a)

> +          && evolution_function_right_is_integer_cst (chrec_a)

>            && evolution_function_is_affine_multivariate_p (chrec_b, loop_nest->num)

> -          && !chrec_contains_symbols (chrec_b))

> +          && !chrec_contains_symbols (chrec_b)

> +          && evolution_function_right_is_integer_cst (chrec_b))

>      {

>        /* testsuite/.../ssa-chrec-35.c

>          {0, +, 1}_2  vs.  {0, +, 1}_3
Richard Sandiford Jan. 5, 2018, 11:02 a.m. UTC | #2
Richard Biener <richard.guenther@gmail.com> writes:
> On Mon, Nov 6, 2017 at 9:12 PM, Richard Sandiford

> <richard.sandiford@linaro.org> wrote:

>> initialize_matrix_A requires the CHREC_RIGHT to be an INTEGER_CST:

>>

>>   switch (TREE_CODE (chrec))

>>     {

>>     case POLYNOMIAL_CHREC:

>>       A[index][0] = mult * int_cst_value (CHREC_RIGHT (chrec));

>>       return initialize_matrix_A (A, CHREC_LEFT (chrec), index + 1, mult);

>>

>> and isn't able to back out if it isn't.  This patch instead

>> checks for an appropriate CHREC_RIGHT before calling the parent

>> function analyze_subscript_affine_affine.

>>

>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.

>> OK to install?

>

> Hmm.  I think this papers over some underlying issues.  The calls are

> guarded already with predicates that are supposed to veryify what you do.

> Namely chrec_contains_symbols (a poly_int _does_ contain a "symbol").


Yeah, I guess.  "Symbol" seems to mean different things in different
places: elsewhere it's !is_gimple_min_invariant, while in rtl it's
specifically things that would have an assembler label.  And "if it's
not a symbol then it must be an INTEGER_CST" seems a bit dangerous in
general, which I thought was why we also had
evolution_function_right_is_integer_cst.

E.g. tree-ssa-loop-ivcanon.c uses chrec_contains_symbols to test whether
something is likely to be folded to a constant, and N * POLY_INT_CST
would be folded to a POLY_INT_CST.  So in that kind of usage we'd
want chrec_contains_symbols to be false.

But...

> So can you please fix that by adding TREE_CODE (chrec) == POLY_INT

> to the return true case?


...I don't have any proof that that causes problems in practice,
so is the following OK?

Thanks,
Richard


2018-01-05  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	* tree-chrec.c (chrec_contains_symbols): Return true for
	POLY_INT_CST.

Index: gcc/tree-chrec.c
===================================================================
--- gcc/tree-chrec.c	2018-01-03 11:12:56.820720031 +0000
+++ gcc/tree-chrec.c	2018-01-05 11:01:00.520570935 +0000
@@ -961,6 +961,7 @@ chrec_contains_symbols (const_tree chrec
 
   if (TREE_CODE (chrec) == SSA_NAME
       || VAR_P (chrec)
+      || TREE_CODE (chrec) == POLY_INT_CST
       || TREE_CODE (chrec) == PARM_DECL
       || TREE_CODE (chrec) == FUNCTION_DECL
       || TREE_CODE (chrec) == LABEL_DECL
Richard Biener Jan. 5, 2018, 11:38 a.m. UTC | #3
On Fri, Jan 5, 2018 at 12:02 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:

>> On Mon, Nov 6, 2017 at 9:12 PM, Richard Sandiford

>> <richard.sandiford@linaro.org> wrote:

>>> initialize_matrix_A requires the CHREC_RIGHT to be an INTEGER_CST:

>>>

>>>   switch (TREE_CODE (chrec))

>>>     {

>>>     case POLYNOMIAL_CHREC:

>>>       A[index][0] = mult * int_cst_value (CHREC_RIGHT (chrec));

>>>       return initialize_matrix_A (A, CHREC_LEFT (chrec), index + 1, mult);

>>>

>>> and isn't able to back out if it isn't.  This patch instead

>>> checks for an appropriate CHREC_RIGHT before calling the parent

>>> function analyze_subscript_affine_affine.

>>>

>>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.

>>> OK to install?

>>

>> Hmm.  I think this papers over some underlying issues.  The calls are

>> guarded already with predicates that are supposed to veryify what you do.

>> Namely chrec_contains_symbols (a poly_int _does_ contain a "symbol").

>

> Yeah, I guess.  "Symbol" seems to mean different things in different

> places: elsewhere it's !is_gimple_min_invariant, while in rtl it's

> specifically things that would have an assembler label.  And "if it's

> not a symbol then it must be an INTEGER_CST" seems a bit dangerous in

> general, which I thought was why we also had

> evolution_function_right_is_integer_cst.


Yeah, well ... we do have quite some (too) many predicates here.

> E.g. tree-ssa-loop-ivcanon.c uses chrec_contains_symbols to test whether

> something is likely to be folded to a constant, and N * POLY_INT_CST

> would be folded to a POLY_INT_CST.  So in that kind of usage we'd

> want chrec_contains_symbols to be false.

>

> But...

>

>> So can you please fix that by adding TREE_CODE (chrec) == POLY_INT

>> to the return true case?

>

> ...I don't have any proof that that causes problems in practice,

> so is the following OK?


Yes.

Thanks,
Richard.

> Thanks,

> Richard

>

>

> 2018-01-05  Richard Sandiford  <richard.sandiford@linaro.org>

>

> gcc/

>         * tree-chrec.c (chrec_contains_symbols): Return true for

>         POLY_INT_CST.

>

> Index: gcc/tree-chrec.c

> ===================================================================

> --- gcc/tree-chrec.c    2018-01-03 11:12:56.820720031 +0000

> +++ gcc/tree-chrec.c    2018-01-05 11:01:00.520570935 +0000

> @@ -961,6 +961,7 @@ chrec_contains_symbols (const_tree chrec

>

>    if (TREE_CODE (chrec) == SSA_NAME

>        || VAR_P (chrec)

> +      || TREE_CODE (chrec) == POLY_INT_CST

>        || TREE_CODE (chrec) == PARM_DECL

>        || TREE_CODE (chrec) == FUNCTION_DECL

>        || TREE_CODE (chrec) == LABEL_DECL
diff mbox series

Patch

Index: gcc/tree-data-ref.c
===================================================================
--- gcc/tree-data-ref.c	2017-11-06 19:39:32.924734707 +0000
+++ gcc/tree-data-ref.c	2017-11-06 20:10:40.704478657 +0000
@@ -3788,7 +3788,9 @@  analyze_siv_subscript (tree chrec_a,
 				      overlaps_b, overlaps_a, last_conflicts);
 
   else if (evolution_function_is_affine_in_loop (chrec_a, loop_nest_num)
-	   && evolution_function_is_affine_in_loop (chrec_b, loop_nest_num))
+	   && evolution_function_right_is_integer_cst (chrec_a)
+	   && evolution_function_is_affine_in_loop (chrec_b, loop_nest_num)
+	   && evolution_function_right_is_integer_cst (chrec_b))
     {
       if (!chrec_contains_symbols (chrec_a)
 	  && !chrec_contains_symbols (chrec_b))
@@ -3922,8 +3924,10 @@  analyze_miv_subscript (tree chrec_a,
 
   else if (evolution_function_is_affine_multivariate_p (chrec_a, loop_nest->num)
 	   && !chrec_contains_symbols (chrec_a)
+	   && evolution_function_right_is_integer_cst (chrec_a)
 	   && evolution_function_is_affine_multivariate_p (chrec_b, loop_nest->num)
-	   && !chrec_contains_symbols (chrec_b))
+	   && !chrec_contains_symbols (chrec_b)
+	   && evolution_function_right_is_integer_cst (chrec_b))
     {
       /* testsuite/.../ssa-chrec-35.c
 	 {0, +, 1}_2  vs.  {0, +, 1}_3