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