* [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] @ 2024-07-01 20:13 Tamar Christina 2024-07-01 20:14 ` [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] Tamar Christina ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Tamar Christina @ 2024-07-01 20:13 UTC (permalink / raw) To: gcc-patches; +Cc: nd, rguenther, jlaw [-- Attachment #1: Type: text/plain, Size: 1782 bytes --] Hi All, wide_int_constant_multiple_p tries to check if for two tree expressions a and b that there is a multiplier which makes a == b * c. This code however seems to think that there's no c where a=0 and b=0 are equal which is of course wrong. This fixes it and also fixes the comment. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu -m32, -m64 and no issues. Ok for master? Thanks, Tamar gcc/ChangeLog: PR tree-optimization/114932 * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being multiples. --- diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc index d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca38c9ef632fcba 100644 --- a/gcc/tree-affine.cc +++ b/gcc/tree-affine.cc @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, name_expansion *> **cache) *cache = NULL; } -/* If VAL != CST * DIV for any constant CST, returns false. - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, - and if they are different, returns false. Finally, if neither of these - two cases occur, true is returned, and CST is stored to MULT and MULT_SET - is set to true. */ +/* If VAL == CST * DIV for any constant CST, returns true. + and if *MULT_SET is true, additionally compares CST and MULT + and if they are different, returns false. If true is returned, CST is + stored to MULT and MULT_SET is set to true. */ static bool wide_int_constant_multiple_p (const poly_widest_int &val, @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int &val, if (known_eq (val, 0)) { + if (maybe_eq (div, 0)) + { + *mult = 1; + return true; + } + if (*mult_set && maybe_ne (*mult, 0)) return false; *mult_set = true; -- [-- Attachment #2: rb18602.patch --] [-- Type: text/x-diff, Size: 1225 bytes --] diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc index d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca38c9ef632fcba 100644 --- a/gcc/tree-affine.cc +++ b/gcc/tree-affine.cc @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, name_expansion *> **cache) *cache = NULL; } -/* If VAL != CST * DIV for any constant CST, returns false. - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, - and if they are different, returns false. Finally, if neither of these - two cases occur, true is returned, and CST is stored to MULT and MULT_SET - is set to true. */ +/* If VAL == CST * DIV for any constant CST, returns true. + and if *MULT_SET is true, additionally compares CST and MULT + and if they are different, returns false. If true is returned, CST is + stored to MULT and MULT_SET is set to true. */ static bool wide_int_constant_multiple_p (const poly_widest_int &val, @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int &val, if (known_eq (val, 0)) { + if (maybe_eq (div, 0)) + { + *mult = 1; + return true; + } + if (*mult_set && maybe_ne (*mult, 0)) return false; *mult_set = true; ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] 2024-07-01 20:13 [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina @ 2024-07-01 20:14 ` Tamar Christina 2024-07-02 7:58 ` Richard Biener 2024-07-01 20:32 ` [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina 2024-07-02 7:56 ` Richard Biener 2 siblings, 1 reply; 11+ messages in thread From: Tamar Christina @ 2024-07-01 20:14 UTC (permalink / raw) To: gcc-patches; +Cc: nd, rguenther, jlaw [-- Attachment #1: Type: text/plain, Size: 3075 bytes --] Hi All, The current implementation of constant_multiple_of is doing a more limited version of aff_combination_constant_multiple_p. The only non-debug usage of constant_multiple_of will proceed with the values as affine trees. There is scope for further optimization here, namely I believe that if constant_multiple_of returns the aff_tree after the conversion then get_computation_aff_1 can use it instead of manually creating the aff_tree. However I think it makes sense to first commit this smaller change and then incrementally change things. Bootstrapped Regtested on aarch64-none-linux-gnu, x86_64-pc-linux-gnu -m32, -m64 and no issues. Ok for master? Thanks, Tamar gcc/ChangeLog: PR tree-optimization/114932 * tree-ssa-loop-ivopts.cc (constant_multiple_of): Use aff_combination_constant_multiple_p instead. --- diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc index 7cae5bdefea3648ddde238a357af527a934a569e..c3218a3e8eedbb8d0a7f14c01eeb069cb6024c29 100644 --- a/gcc/tree-ssa-loop-ivopts.cc +++ b/gcc/tree-ssa-loop-ivopts.cc @@ -2146,65 +2146,15 @@ idx_record_use (tree base, tree *idx, static bool constant_multiple_of (tree top, tree bot, widest_int *mul) { - tree mby; - enum tree_code code; - unsigned precision = TYPE_PRECISION (TREE_TYPE (top)); - widest_int res, p0, p1; - - STRIP_NOPS (top); - STRIP_NOPS (bot); - - if (operand_equal_p (top, bot, 0)) - { - *mul = 1; - return true; - } - - code = TREE_CODE (top); - switch (code) - { - case MULT_EXPR: - mby = TREE_OPERAND (top, 1); - if (TREE_CODE (mby) != INTEGER_CST) - return false; - - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res)) - return false; - - *mul = wi::sext (res * wi::to_widest (mby), precision); - return true; - - case PLUS_EXPR: - case MINUS_EXPR: - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0) - || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1)) - return false; - - if (code == MINUS_EXPR) - p1 = -p1; - *mul = wi::sext (p0 + p1, precision); - return true; - - case INTEGER_CST: - if (TREE_CODE (bot) != INTEGER_CST) - return false; - - p0 = widest_int::from (wi::to_wide (top), SIGNED); - p1 = widest_int::from (wi::to_wide (bot), SIGNED); - if (p1 == 0) - return false; - *mul = wi::sext (wi::divmod_trunc (p0, p1, SIGNED, &res), precision); - return res == 0; - - default: - if (POLY_INT_CST_P (top) - && POLY_INT_CST_P (bot) - && constant_multiple_p (wi::to_poly_widest (top), - wi::to_poly_widest (bot), mul)) - return true; + aff_tree aff_top, aff_bot; + tree_to_aff_combination (top, TREE_TYPE (top), &aff_top); + tree_to_aff_combination (bot, TREE_TYPE (bot), &aff_bot); + poly_widest_int poly_mul; + if (aff_combination_constant_multiple_p (&aff_top, &aff_bot, &poly_mul) + && poly_mul.is_constant (mul)) + return true; - return false; - } + return false; } /* Return true if memory reference REF with step STEP may be unaligned. */ -- [-- Attachment #2: rb18603.patch --] [-- Type: text/x-diff, Size: 2240 bytes --] diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc index 7cae5bdefea3648ddde238a357af527a934a569e..c3218a3e8eedbb8d0a7f14c01eeb069cb6024c29 100644 --- a/gcc/tree-ssa-loop-ivopts.cc +++ b/gcc/tree-ssa-loop-ivopts.cc @@ -2146,65 +2146,15 @@ idx_record_use (tree base, tree *idx, static bool constant_multiple_of (tree top, tree bot, widest_int *mul) { - tree mby; - enum tree_code code; - unsigned precision = TYPE_PRECISION (TREE_TYPE (top)); - widest_int res, p0, p1; - - STRIP_NOPS (top); - STRIP_NOPS (bot); - - if (operand_equal_p (top, bot, 0)) - { - *mul = 1; - return true; - } - - code = TREE_CODE (top); - switch (code) - { - case MULT_EXPR: - mby = TREE_OPERAND (top, 1); - if (TREE_CODE (mby) != INTEGER_CST) - return false; - - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res)) - return false; - - *mul = wi::sext (res * wi::to_widest (mby), precision); - return true; - - case PLUS_EXPR: - case MINUS_EXPR: - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0) - || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1)) - return false; - - if (code == MINUS_EXPR) - p1 = -p1; - *mul = wi::sext (p0 + p1, precision); - return true; - - case INTEGER_CST: - if (TREE_CODE (bot) != INTEGER_CST) - return false; - - p0 = widest_int::from (wi::to_wide (top), SIGNED); - p1 = widest_int::from (wi::to_wide (bot), SIGNED); - if (p1 == 0) - return false; - *mul = wi::sext (wi::divmod_trunc (p0, p1, SIGNED, &res), precision); - return res == 0; - - default: - if (POLY_INT_CST_P (top) - && POLY_INT_CST_P (bot) - && constant_multiple_p (wi::to_poly_widest (top), - wi::to_poly_widest (bot), mul)) - return true; + aff_tree aff_top, aff_bot; + tree_to_aff_combination (top, TREE_TYPE (top), &aff_top); + tree_to_aff_combination (bot, TREE_TYPE (bot), &aff_bot); + poly_widest_int poly_mul; + if (aff_combination_constant_multiple_p (&aff_top, &aff_bot, &poly_mul) + && poly_mul.is_constant (mul)) + return true; - return false; - } + return false; } /* Return true if memory reference REF with step STEP may be unaligned. */ ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] 2024-07-01 20:14 ` [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] Tamar Christina @ 2024-07-02 7:58 ` Richard Biener 0 siblings, 0 replies; 11+ messages in thread From: Richard Biener @ 2024-07-02 7:58 UTC (permalink / raw) To: Tamar Christina; +Cc: gcc-patches, nd, jlaw On Mon, 1 Jul 2024, Tamar Christina wrote: > Hi All, > > The current implementation of constant_multiple_of is doing a more limited > version of aff_combination_constant_multiple_p. > > The only non-debug usage of constant_multiple_of will proceed with the values > as affine trees. There is scope for further optimization here, namely I believe > that if constant_multiple_of returns the aff_tree after the conversion then > get_computation_aff_1 can use it instead of manually creating the aff_tree. > > However I think it makes sense to first commit this smaller change and then > incrementally change things. > > Bootstrapped Regtested on aarch64-none-linux-gnu, > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > Ok for master? OK. Thanks, Richard. > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/114932 > * tree-ssa-loop-ivopts.cc (constant_multiple_of): Use > aff_combination_constant_multiple_p instead. > > --- > diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc > index 7cae5bdefea3648ddde238a357af527a934a569e..c3218a3e8eedbb8d0a7f14c01eeb069cb6024c29 100644 > --- a/gcc/tree-ssa-loop-ivopts.cc > +++ b/gcc/tree-ssa-loop-ivopts.cc > @@ -2146,65 +2146,15 @@ idx_record_use (tree base, tree *idx, > static bool > constant_multiple_of (tree top, tree bot, widest_int *mul) > { > - tree mby; > - enum tree_code code; > - unsigned precision = TYPE_PRECISION (TREE_TYPE (top)); > - widest_int res, p0, p1; > - > - STRIP_NOPS (top); > - STRIP_NOPS (bot); > - > - if (operand_equal_p (top, bot, 0)) > - { > - *mul = 1; > - return true; > - } > - > - code = TREE_CODE (top); > - switch (code) > - { > - case MULT_EXPR: > - mby = TREE_OPERAND (top, 1); > - if (TREE_CODE (mby) != INTEGER_CST) > - return false; > - > - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &res)) > - return false; > - > - *mul = wi::sext (res * wi::to_widest (mby), precision); > - return true; > - > - case PLUS_EXPR: > - case MINUS_EXPR: > - if (!constant_multiple_of (TREE_OPERAND (top, 0), bot, &p0) > - || !constant_multiple_of (TREE_OPERAND (top, 1), bot, &p1)) > - return false; > - > - if (code == MINUS_EXPR) > - p1 = -p1; > - *mul = wi::sext (p0 + p1, precision); > - return true; > - > - case INTEGER_CST: > - if (TREE_CODE (bot) != INTEGER_CST) > - return false; > - > - p0 = widest_int::from (wi::to_wide (top), SIGNED); > - p1 = widest_int::from (wi::to_wide (bot), SIGNED); > - if (p1 == 0) > - return false; > - *mul = wi::sext (wi::divmod_trunc (p0, p1, SIGNED, &res), precision); > - return res == 0; > - > - default: > - if (POLY_INT_CST_P (top) > - && POLY_INT_CST_P (bot) > - && constant_multiple_p (wi::to_poly_widest (top), > - wi::to_poly_widest (bot), mul)) > - return true; > + aff_tree aff_top, aff_bot; > + tree_to_aff_combination (top, TREE_TYPE (top), &aff_top); > + tree_to_aff_combination (bot, TREE_TYPE (bot), &aff_bot); > + poly_widest_int poly_mul; > + if (aff_combination_constant_multiple_p (&aff_top, &aff_bot, &poly_mul) > + && poly_mul.is_constant (mul)) > + return true; > > - return false; > - } > + return false; > } > > /* Return true if memory reference REF with step STEP may be unaligned. */ > > > > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-01 20:13 [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina 2024-07-01 20:14 ` [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] Tamar Christina @ 2024-07-01 20:32 ` Tamar Christina 2024-07-02 8:01 ` Richard Biener 2024-07-02 7:56 ` Richard Biener 2 siblings, 1 reply; 11+ messages in thread From: Tamar Christina @ 2024-07-01 20:32 UTC (permalink / raw) To: Tamar Christina, gcc-patches; +Cc: nd, rguenther, jlaw > -----Original Message----- > From: Tamar Christina <tamar.christina@arm.com> > Sent: Monday, July 1, 2024 9:14 PM > To: gcc-patches@gcc.gnu.org > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > DIV are 0. [PR114932] > > Hi All, > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > that there is a multiplier which makes a == b * c. > > This code however seems to think that there's no c where a=0 and b=0 are equal > which is of course wrong. > > This fixes it and also fixes the comment. > > Bootstrapped Regtested on aarch64-none-linux-gnu, > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/114932 > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > multiples. > > --- > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > index > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > 38c9ef632fcba 100644 > --- a/gcc/tree-affine.cc > +++ b/gcc/tree-affine.cc > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > name_expansion *> **cache) > *cache = NULL; > } > > -/* If VAL != CST * DIV for any constant CST, returns false. > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > - and if they are different, returns false. Finally, if neither of these > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > - is set to true. */ > +/* If VAL == CST * DIV for any constant CST, returns true. > + and if *MULT_SET is true, additionally compares CST and MULT > + and if they are different, returns false. If true is returned, CST is > + stored to MULT and MULT_SET is set to true. */ > > static bool > wide_int_constant_multiple_p (const poly_widest_int &val, > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > &val, > > if (known_eq (val, 0)) > { > + if (maybe_eq (div, 0)) > + { > + *mult = 1; > + return true; > + } > + Note, I also tested known_eq here, and also no regression on what I can test. I picked maybe_eq since that's what the lines after this one tests. I'm not sure I fully understand why one tests known and the other maybe. It seems to me that both should test known. But I tested both so which ever one is felt to be more correct I can commit If ok. Thanks, Tamar > if (*mult_set && maybe_ne (*mult, 0)) > return false; > *mult_set = true; > > > > > -- ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-01 20:32 ` [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina @ 2024-07-02 8:01 ` Richard Biener 2024-07-02 9:46 ` Alex Coplan 0 siblings, 1 reply; 11+ messages in thread From: Richard Biener @ 2024-07-02 8:01 UTC (permalink / raw) To: Tamar Christina; +Cc: gcc-patches, nd, jlaw On Mon, 1 Jul 2024, Tamar Christina wrote: > > -----Original Message----- > > From: Tamar Christina <tamar.christina@arm.com> > > Sent: Monday, July 1, 2024 9:14 PM > > To: gcc-patches@gcc.gnu.org > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > > DIV are 0. [PR114932] > > > > Hi All, > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > > that there is a multiplier which makes a == b * c. > > > > This code however seems to think that there's no c where a=0 and b=0 are equal > > which is of course wrong. > > > > This fixes it and also fixes the comment. > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > > > Ok for master? > > > > Thanks, > > Tamar > > > > gcc/ChangeLog: > > > > PR tree-optimization/114932 > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > > multiples. > > > > --- > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > > index > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > > 38c9ef632fcba 100644 > > --- a/gcc/tree-affine.cc > > +++ b/gcc/tree-affine.cc > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > > name_expansion *> **cache) > > *cache = NULL; > > } > > > > -/* If VAL != CST * DIV for any constant CST, returns false. > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > > - and if they are different, returns false. Finally, if neither of these > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > > - is set to true. */ > > +/* If VAL == CST * DIV for any constant CST, returns true. > > + and if *MULT_SET is true, additionally compares CST and MULT > > + and if they are different, returns false. If true is returned, CST is > > + stored to MULT and MULT_SET is set to true. */ > > > > static bool > > wide_int_constant_multiple_p (const poly_widest_int &val, > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > > &val, > > > > if (known_eq (val, 0)) > > { > > + if (maybe_eq (div, 0)) > > + { > > + *mult = 1; > > + return true; > > + } > > + > > Note, I also tested known_eq here, and also no regression on what I can test. > I picked maybe_eq since that's what the lines after this one tests. I think the maybe_eq (div, 0) is because otherwise multiple_p might crash? I'm not sure if there's a difference between maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq POLY_INT look like that's not known_eq? > I'm not sure I fully understand why one tests known and the other maybe. It seems to me > that both should test known. But I tested both so which ever one is felt to be more correct > I can commit If ok. > > Thanks, > Tamar > > > if (*mult_set && maybe_ne (*mult, 0)) > > return false; > > *mult_set = true; > > > > > > > > > > -- > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-02 8:01 ` Richard Biener @ 2024-07-02 9:46 ` Alex Coplan 2024-07-02 10:17 ` Alex Coplan 0 siblings, 1 reply; 11+ messages in thread From: Alex Coplan @ 2024-07-02 9:46 UTC (permalink / raw) To: Richard Biener; +Cc: Tamar Christina, gcc-patches, nd, jlaw On 02/07/2024 10:01, Richard Biener wrote: > On Mon, 1 Jul 2024, Tamar Christina wrote: > > > > -----Original Message----- > > > From: Tamar Christina <tamar.christina@arm.com> > > > Sent: Monday, July 1, 2024 9:14 PM > > > To: gcc-patches@gcc.gnu.org > > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > > > DIV are 0. [PR114932] > > > > > > Hi All, > > > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > > > that there is a multiplier which makes a == b * c. > > > > > > This code however seems to think that there's no c where a=0 and b=0 are equal > > > which is of course wrong. > > > > > > This fixes it and also fixes the comment. > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > > > > > Ok for master? > > > > > > Thanks, > > > Tamar > > > > > > gcc/ChangeLog: > > > > > > PR tree-optimization/114932 > > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > > > multiples. > > > > > > --- > > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > > > index > > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > > > 38c9ef632fcba 100644 > > > --- a/gcc/tree-affine.cc > > > +++ b/gcc/tree-affine.cc > > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > > > name_expansion *> **cache) > > > *cache = NULL; > > > } > > > > > > -/* If VAL != CST * DIV for any constant CST, returns false. > > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > > > - and if they are different, returns false. Finally, if neither of these > > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > > > - is set to true. */ > > > +/* If VAL == CST * DIV for any constant CST, returns true. > > > + and if *MULT_SET is true, additionally compares CST and MULT > > > + and if they are different, returns false. If true is returned, CST is > > > + stored to MULT and MULT_SET is set to true. */ > > > > > > static bool > > > wide_int_constant_multiple_p (const poly_widest_int &val, > > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > > > &val, > > > > > > if (known_eq (val, 0)) > > > { > > > + if (maybe_eq (div, 0)) > > > + { > > > + *mult = 1; > > > + return true; > > > + } > > > + > > > > Note, I also tested known_eq here, and also no regression on what I can test. > > I picked maybe_eq since that's what the lines after this one tests. > > I think the maybe_eq (div, 0) is because otherwise multiple_p might > crash? I'm not sure if there's a difference between > maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq > POLY_INT look like that's not known_eq? Take: A = POLY_INT_CST [16,0] B = POLY_INT_CST [8,8] then these represent polynomials: A = 16 B = 8 + 8x where x is only known at runtime. We have maybe_eq (A,B) since there is a value of x (= 1) which makes these equal at runtime, but clearly !known_eq (A,B) (take x = 0, for example). That is my understanding at least, hopefully that makes sense. Thanks, Alex > > > I'm not sure I fully understand why one tests known and the other maybe. It seems to me > > that both should test known. But I tested both so which ever one is felt to be more correct > > I can commit If ok. > > > > Thanks, > > Tamar > > > > > if (*mult_set && maybe_ne (*mult, 0)) > > > return false; > > > *mult_set = true; > > > > > > > > > > > > > > > -- > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, > Frankenstrasse 146, 90461 Nuernberg, Germany; > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-02 9:46 ` Alex Coplan @ 2024-07-02 10:17 ` Alex Coplan 2024-07-02 11:41 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Alex Coplan @ 2024-07-02 10:17 UTC (permalink / raw) To: Richard Biener; +Cc: Tamar Christina, gcc-patches, nd, jlaw On 02/07/2024 10:46, Alex Coplan wrote: > On 02/07/2024 10:01, Richard Biener wrote: > > On Mon, 1 Jul 2024, Tamar Christina wrote: > > > > > > -----Original Message----- > > > > From: Tamar Christina <tamar.christina@arm.com> > > > > Sent: Monday, July 1, 2024 9:14 PM > > > > To: gcc-patches@gcc.gnu.org > > > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > > > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > > > > DIV are 0. [PR114932] > > > > > > > > Hi All, > > > > > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > > > > that there is a multiplier which makes a == b * c. > > > > > > > > This code however seems to think that there's no c where a=0 and b=0 are equal > > > > which is of course wrong. > > > > > > > > This fixes it and also fixes the comment. > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > > > > > > > Ok for master? > > > > > > > > Thanks, > > > > Tamar > > > > > > > > gcc/ChangeLog: > > > > > > > > PR tree-optimization/114932 > > > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > > > > multiples. > > > > > > > > --- > > > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > > > > index > > > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > > > > 38c9ef632fcba 100644 > > > > --- a/gcc/tree-affine.cc > > > > +++ b/gcc/tree-affine.cc > > > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > > > > name_expansion *> **cache) > > > > *cache = NULL; > > > > } > > > > > > > > -/* If VAL != CST * DIV for any constant CST, returns false. > > > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > > > > - and if they are different, returns false. Finally, if neither of these > > > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > > > > - is set to true. */ > > > > +/* If VAL == CST * DIV for any constant CST, returns true. > > > > + and if *MULT_SET is true, additionally compares CST and MULT > > > > + and if they are different, returns false. If true is returned, CST is > > > > + stored to MULT and MULT_SET is set to true. */ > > > > > > > > static bool > > > > wide_int_constant_multiple_p (const poly_widest_int &val, > > > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > > > > &val, > > > > > > > > if (known_eq (val, 0)) > > > > { > > > > + if (maybe_eq (div, 0)) > > > > + { > > > > + *mult = 1; > > > > + return true; > > > > + } > > > > + > > > > > > Note, I also tested known_eq here, and also no regression on what I can test. > > > I picked maybe_eq since that's what the lines after this one tests. > > > > I think the maybe_eq (div, 0) is because otherwise multiple_p might > > crash? I'm not sure if there's a difference between > > maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq > > POLY_INT look like that's not known_eq? > > Take: > > A = POLY_INT_CST [16,0] > B = POLY_INT_CST [8,8] > > then these represent polynomials: > > A = 16 > B = 8 + 8x > > where x is only known at runtime. We have maybe_eq (A,B) since there is > a value of x (= 1) which makes these equal at runtime, but clearly > !known_eq (A,B) (take x = 0, for example). So specifically in the case of: maybe_eq (x, 0) vs known_eq (x, 0) I suppose x = POLY_INT_CST [-4,4] would satisfy the first (again with x = 1) but not the second. Thanks, Alex > > That is my understanding at least, hopefully that makes sense. > > Thanks, > Alex > > > > > > I'm not sure I fully understand why one tests known and the other maybe. It seems to me > > > that both should test known. But I tested both so which ever one is felt to be more correct > > > I can commit If ok. > > > > > > Thanks, > > > Tamar > > > > > > > if (*mult_set && maybe_ne (*mult, 0)) > > > > return false; > > > > *mult_set = true; > > > > > > > > > > > > > > > > > > > > -- > > > > > > > -- > > Richard Biener <rguenther@suse.de> > > SUSE Software Solutions Germany GmbH, > > Frankenstrasse 146, 90461 Nuernberg, Germany; > > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-02 10:17 ` Alex Coplan @ 2024-07-02 11:41 ` Richard Biener 2024-07-02 13:36 ` Alex Coplan 0 siblings, 1 reply; 11+ messages in thread From: Richard Biener @ 2024-07-02 11:41 UTC (permalink / raw) To: Alex Coplan; +Cc: Tamar Christina, gcc-patches, nd, jlaw On Tue, 2 Jul 2024, Alex Coplan wrote: > On 02/07/2024 10:46, Alex Coplan wrote: > > On 02/07/2024 10:01, Richard Biener wrote: > > > On Mon, 1 Jul 2024, Tamar Christina wrote: > > > > > > > > -----Original Message----- > > > > > From: Tamar Christina <tamar.christina@arm.com> > > > > > Sent: Monday, July 1, 2024 9:14 PM > > > > > To: gcc-patches@gcc.gnu.org > > > > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > > > > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > > > > > DIV are 0. [PR114932] > > > > > > > > > > Hi All, > > > > > > > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > > > > > that there is a multiplier which makes a == b * c. > > > > > > > > > > This code however seems to think that there's no c where a=0 and b=0 are equal > > > > > which is of course wrong. > > > > > > > > > > This fixes it and also fixes the comment. > > > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > > > > > > > > > Ok for master? > > > > > > > > > > Thanks, > > > > > Tamar > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > PR tree-optimization/114932 > > > > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > > > > > multiples. > > > > > > > > > > --- > > > > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > > > > > index > > > > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > > > > > 38c9ef632fcba 100644 > > > > > --- a/gcc/tree-affine.cc > > > > > +++ b/gcc/tree-affine.cc > > > > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > > > > > name_expansion *> **cache) > > > > > *cache = NULL; > > > > > } > > > > > > > > > > -/* If VAL != CST * DIV for any constant CST, returns false. > > > > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > > > > > - and if they are different, returns false. Finally, if neither of these > > > > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > > > > > - is set to true. */ > > > > > +/* If VAL == CST * DIV for any constant CST, returns true. > > > > > + and if *MULT_SET is true, additionally compares CST and MULT > > > > > + and if they are different, returns false. If true is returned, CST is > > > > > + stored to MULT and MULT_SET is set to true. */ > > > > > > > > > > static bool > > > > > wide_int_constant_multiple_p (const poly_widest_int &val, > > > > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > > > > > &val, > > > > > > > > > > if (known_eq (val, 0)) > > > > > { > > > > > + if (maybe_eq (div, 0)) > > > > > + { > > > > > + *mult = 1; > > > > > + return true; > > > > > + } > > > > > + > > > > > > > > Note, I also tested known_eq here, and also no regression on what I can test. > > > > I picked maybe_eq since that's what the lines after this one tests. > > > > > > I think the maybe_eq (div, 0) is because otherwise multiple_p might > > > crash? I'm not sure if there's a difference between > > > maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq > > > POLY_INT look like that's not known_eq? > > > > Take: > > > > A = POLY_INT_CST [16,0] > > B = POLY_INT_CST [8,8] > > > > then these represent polynomials: > > > > A = 16 > > B = 8 + 8x > > > > where x is only known at runtime. We have maybe_eq (A,B) since there is > > a value of x (= 1) which makes these equal at runtime, but clearly > > !known_eq (A,B) (take x = 0, for example). > > So specifically in the case of: > > maybe_eq (x, 0) vs known_eq (x, 0) > > I suppose x = POLY_INT_CST [-4,4] would satisfy the first (again with x > = 1) but not the second. Ah yeah - I wasn't aware that a negative offset is a thing. I think that at least we know x > 0, right, so [0, 4] is never zero, likewise [4, 4] never is? Richard. > Thanks, > Alex > > > > > That is my understanding at least, hopefully that makes sense. > > > > Thanks, > > Alex > > > > > > > > > I'm not sure I fully understand why one tests known and the other maybe. It seems to me > > > > that both should test known. But I tested both so which ever one is felt to be more correct > > > > I can commit If ok. > > > > > > > > Thanks, > > > > Tamar > > > > > > > > > if (*mult_set && maybe_ne (*mult, 0)) > > > > > return false; > > > > > *mult_set = true; > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > -- > > > Richard Biener <rguenther@suse.de> > > > SUSE Software Solutions Germany GmbH, > > > Frankenstrasse 146, 90461 Nuernberg, Germany; > > > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-02 11:41 ` Richard Biener @ 2024-07-02 13:36 ` Alex Coplan 2024-07-02 20:00 ` Richard Sandiford 0 siblings, 1 reply; 11+ messages in thread From: Alex Coplan @ 2024-07-02 13:36 UTC (permalink / raw) To: Richard Biener; +Cc: Tamar Christina, gcc-patches, nd, jlaw On 02/07/2024 13:41, Richard Biener wrote: > On Tue, 2 Jul 2024, Alex Coplan wrote: > > > On 02/07/2024 10:46, Alex Coplan wrote: > > > On 02/07/2024 10:01, Richard Biener wrote: > > > > On Mon, 1 Jul 2024, Tamar Christina wrote: > > > > > > > > > > -----Original Message----- > > > > > > From: Tamar Christina <tamar.christina@arm.com> > > > > > > Sent: Monday, July 1, 2024 9:14 PM > > > > > > To: gcc-patches@gcc.gnu.org > > > > > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com > > > > > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and > > > > > > DIV are 0. [PR114932] > > > > > > > > > > > > Hi All, > > > > > > > > > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > > > > > > that there is a multiplier which makes a == b * c. > > > > > > > > > > > > This code however seems to think that there's no c where a=0 and b=0 are equal > > > > > > which is of course wrong. > > > > > > > > > > > > This fixes it and also fixes the comment. > > > > > > > > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, > > > > > > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > > > > > > > > > > > Ok for master? > > > > > > > > > > > > Thanks, > > > > > > Tamar > > > > > > > > > > > > gcc/ChangeLog: > > > > > > > > > > > > PR tree-optimization/114932 > > > > > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > > > > > > multiples. > > > > > > > > > > > > --- > > > > > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > > > > > > index > > > > > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca > > > > > > 38c9ef632fcba 100644 > > > > > > --- a/gcc/tree-affine.cc > > > > > > +++ b/gcc/tree-affine.cc > > > > > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, > > > > > > name_expansion *> **cache) > > > > > > *cache = NULL; > > > > > > } > > > > > > > > > > > > -/* If VAL != CST * DIV for any constant CST, returns false. > > > > > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > > > > > > - and if they are different, returns false. Finally, if neither of these > > > > > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > > > > > > - is set to true. */ > > > > > > +/* If VAL == CST * DIV for any constant CST, returns true. > > > > > > + and if *MULT_SET is true, additionally compares CST and MULT > > > > > > + and if they are different, returns false. If true is returned, CST is > > > > > > + stored to MULT and MULT_SET is set to true. */ > > > > > > > > > > > > static bool > > > > > > wide_int_constant_multiple_p (const poly_widest_int &val, > > > > > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int > > > > > > &val, > > > > > > > > > > > > if (known_eq (val, 0)) > > > > > > { > > > > > > + if (maybe_eq (div, 0)) > > > > > > + { > > > > > > + *mult = 1; > > > > > > + return true; > > > > > > + } > > > > > > + > > > > > > > > > > Note, I also tested known_eq here, and also no regression on what I can test. > > > > > I picked maybe_eq since that's what the lines after this one tests. > > > > > > > > I think the maybe_eq (div, 0) is because otherwise multiple_p might > > > > crash? I'm not sure if there's a difference between > > > > maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq > > > > POLY_INT look like that's not known_eq? > > > > > > Take: > > > > > > A = POLY_INT_CST [16,0] > > > B = POLY_INT_CST [8,8] > > > > > > then these represent polynomials: > > > > > > A = 16 > > > B = 8 + 8x > > > > > > where x is only known at runtime. We have maybe_eq (A,B) since there is > > > a value of x (= 1) which makes these equal at runtime, but clearly > > > !known_eq (A,B) (take x = 0, for example). > > > > So specifically in the case of: > > > > maybe_eq (x, 0) vs known_eq (x, 0) > > > > I suppose x = POLY_INT_CST [-4,4] would satisfy the first (again with x > > = 1) but not the second. > > Ah yeah - I wasn't aware that a negative offset is a thing. I think > that at least we know x > 0, right, so [0, 4] is never zero, likewise > [4, 4] never is? I don't think so, I think the only guarantee is that the x >= 0. From doc/poly-int.texi: @code{poly_int} makes the simplifying requirement that each indeterminate must be a nonnegative integer. For SVE the unknown x is the number of 128-bit blocks beyond the minimum of 128, so in particular the indeterminate x = 0 for 128-bit SVE, and we would have [0,4] = 0 and [4,4] = 4 at runtime in that case. Thanks, Alex > > Richard. > > > Thanks, > > Alex > > > > > > > > That is my understanding at least, hopefully that makes sense. > > > > > > Thanks, > > > Alex > > > > > > > > > > > > I'm not sure I fully understand why one tests known and the other maybe. It seems to me > > > > > that both should test known. But I tested both so which ever one is felt to be more correct > > > > > I can commit If ok. > > > > > > > > > > Thanks, > > > > > Tamar > > > > > > > > > > > if (*mult_set && maybe_ne (*mult, 0)) > > > > > > return false; > > > > > > *mult_set = true; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > > > -- > > > > Richard Biener <rguenther@suse.de> > > > > SUSE Software Solutions Germany GmbH, > > > > Frankenstrasse 146, 90461 Nuernberg, Germany; > > > > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, > Frankenstrasse 146, 90461 Nuernberg, Germany; > GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-02 13:36 ` Alex Coplan @ 2024-07-02 20:00 ` Richard Sandiford 0 siblings, 0 replies; 11+ messages in thread From: Richard Sandiford @ 2024-07-02 20:00 UTC (permalink / raw) To: Alex Coplan; +Cc: Richard Biener, Tamar Christina, gcc-patches, nd, jlaw Alex Coplan <alex.coplan@arm.com> writes: > On 02/07/2024 13:41, Richard Biener wrote: >> On Tue, 2 Jul 2024, Alex Coplan wrote: >> >> > On 02/07/2024 10:46, Alex Coplan wrote: >> > > On 02/07/2024 10:01, Richard Biener wrote: >> > > > On Mon, 1 Jul 2024, Tamar Christina wrote: >> > > > >> > > > > > -----Original Message----- >> > > > > > From: Tamar Christina <tamar.christina@arm.com> >> > > > > > Sent: Monday, July 1, 2024 9:14 PM >> > > > > > To: gcc-patches@gcc.gnu.org >> > > > > > Cc: nd <nd@arm.com>; rguenther@suse.de; jlaw@ventanamicro.com >> > > > > > Subject: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and >> > > > > > DIV are 0. [PR114932] >> > > > > > >> > > > > > Hi All, >> > > > > > >> > > > > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b >> > > > > > that there is a multiplier which makes a == b * c. >> > > > > > >> > > > > > This code however seems to think that there's no c where a=0 and b=0 are equal >> > > > > > which is of course wrong. >> > > > > > >> > > > > > This fixes it and also fixes the comment. >> > > > > > >> > > > > > Bootstrapped Regtested on aarch64-none-linux-gnu, >> > > > > > x86_64-pc-linux-gnu -m32, -m64 and no issues. >> > > > > > >> > > > > > Ok for master? >> > > > > > >> > > > > > Thanks, >> > > > > > Tamar >> > > > > > >> > > > > > gcc/ChangeLog: >> > > > > > >> > > > > > PR tree-optimization/114932 >> > > > > > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being >> > > > > > multiples. >> > > > > > >> > > > > > --- >> > > > > > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc >> > > > > > index >> > > > > > d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca >> > > > > > 38c9ef632fcba 100644 >> > > > > > --- a/gcc/tree-affine.cc >> > > > > > +++ b/gcc/tree-affine.cc >> > > > > > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, >> > > > > > name_expansion *> **cache) >> > > > > > *cache = NULL; >> > > > > > } >> > > > > > >> > > > > > -/* If VAL != CST * DIV for any constant CST, returns false. >> > > > > > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, >> > > > > > - and if they are different, returns false. Finally, if neither of these >> > > > > > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET >> > > > > > - is set to true. */ >> > > > > > +/* If VAL == CST * DIV for any constant CST, returns true. >> > > > > > + and if *MULT_SET is true, additionally compares CST and MULT >> > > > > > + and if they are different, returns false. If true is returned, CST is >> > > > > > + stored to MULT and MULT_SET is set to true. */ >> > > > > > >> > > > > > static bool >> > > > > > wide_int_constant_multiple_p (const poly_widest_int &val, >> > > > > > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int >> > > > > > &val, >> > > > > > >> > > > > > if (known_eq (val, 0)) >> > > > > > { >> > > > > > + if (maybe_eq (div, 0)) >> > > > > > + { >> > > > > > + *mult = 1; >> > > > > > + return true; >> > > > > > + } >> > > > > > + >> > > > > >> > > > > Note, I also tested known_eq here, and also no regression on what I can test. >> > > > > I picked maybe_eq since that's what the lines after this one tests. FWIW, the reason for maybe_eq here: if (maybe_eq (div, 0)) return false; if (!multiple_p (val, div, &cst)) return false; is that the division is undefined when div *might* be zero. >> > > > >> > > > I think the maybe_eq (div, 0) is because otherwise multiple_p might >> > > > crash? I'm not sure if there's a difference between >> > > > maybe_eq (x, 0) and known_eq (x, 0) though - how does a maybe_eq >> > > > POLY_INT look like that's not known_eq? >> > > >> > > Take: >> > > >> > > A = POLY_INT_CST [16,0] >> > > B = POLY_INT_CST [8,8] >> > > >> > > then these represent polynomials: >> > > >> > > A = 16 >> > > B = 8 + 8x >> > > >> > > where x is only known at runtime. We have maybe_eq (A,B) since there is >> > > a value of x (= 1) which makes these equal at runtime, but clearly >> > > !known_eq (A,B) (take x = 0, for example). >> > >> > So specifically in the case of: >> > >> > maybe_eq (x, 0) vs known_eq (x, 0) >> > >> > I suppose x = POLY_INT_CST [-4,4] would satisfy the first (again with x >> > = 1) but not the second. >> >> Ah yeah - I wasn't aware that a negative offset is a thing. I think >> that at least we know x > 0, right, so [0, 4] is never zero, likewise >> [4, 4] never is? > > I don't think so, I think the only guarantee is that the > x >= 0. From doc/poly-int.texi: > > @code{poly_int} makes the simplifying requirement that each indeterminate > must be a nonnegative integer. > > For SVE the unknown x is the number of 128-bit blocks beyond the minimum > of 128, so in particular the indeterminate x = 0 for 128-bit SVE, and we > would have [0,4] = 0 and [4,4] = 4 at runtime in that case. Yeah, just wanted to +1 everything Alex said above :) Thanks, Richard ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] 2024-07-01 20:13 [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina 2024-07-01 20:14 ` [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] Tamar Christina 2024-07-01 20:32 ` [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina @ 2024-07-02 7:56 ` Richard Biener 2 siblings, 0 replies; 11+ messages in thread From: Richard Biener @ 2024-07-02 7:56 UTC (permalink / raw) To: Tamar Christina; +Cc: gcc-patches, nd, jlaw On Mon, 1 Jul 2024, Tamar Christina wrote: > Hi All, > > wide_int_constant_multiple_p tries to check if for two tree expressions a and b > that there is a multiplier which makes a == b * c. > > This code however seems to think that there's no c where a=0 and b=0 are equal > which is of course wrong. > > This fixes it and also fixes the comment. > > Bootstrapped Regtested on aarch64-none-linux-gnu, > x86_64-pc-linux-gnu -m32, -m64 and no issues. > > Ok for master? > > Thanks, > Tamar > > gcc/ChangeLog: > > PR tree-optimization/114932 > * tree-affine.cc (wide_int_constant_multiple_p): Support 0 and 0 being > multiples. > > --- > diff --git a/gcc/tree-affine.cc b/gcc/tree-affine.cc > index d6309c4390362b680f0aa97a41fac3281ade66fd..bfea0fe826a6affa0ace154e3ca38c9ef632fcba 100644 > --- a/gcc/tree-affine.cc > +++ b/gcc/tree-affine.cc > @@ -880,11 +880,10 @@ free_affine_expand_cache (hash_map<tree, name_expansion *> **cache) > *cache = NULL; > } > > -/* If VAL != CST * DIV for any constant CST, returns false. > - Otherwise, if *MULT_SET is true, additionally compares CST and MULT, > - and if they are different, returns false. Finally, if neither of these > - two cases occur, true is returned, and CST is stored to MULT and MULT_SET > - is set to true. */ > +/* If VAL == CST * DIV for any constant CST, returns true. > + and if *MULT_SET is true, additionally compares CST and MULT > + and if they are different, returns false. If true is returned, CST is > + stored to MULT and MULT_SET is set to true. */ > > static bool > wide_int_constant_multiple_p (const poly_widest_int &val, > @@ -895,6 +894,12 @@ wide_int_constant_multiple_p (const poly_widest_int &val, > > if (known_eq (val, 0)) > { > + if (maybe_eq (div, 0)) shouldn't that be known_eq as well? > + { > + *mult = 1; and this looks wrong - it shouldn't update *mult if already set, *mult could be 4 (constrained by other sub-expressions). I think it also shouldn't set *mult_set to true. The function comment should mention this exceptional case 0 == CST * O, maybe as "If VAL and DIV are zero then any constant CST satisfies the equality. In this case neither *MUL nor *MULT_SET are updated and the function returns true." OK with that changes. Richard. > + return true; > + } > + > if (*mult_set && maybe_ne (*mult, 0)) > return false; > *mult_set = true; > > > > > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-07-02 20:00 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-07-01 20:13 [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina 2024-07-01 20:14 ` [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932] Tamar Christina 2024-07-02 7:58 ` Richard Biener 2024-07-01 20:32 ` [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932] Tamar Christina 2024-07-02 8:01 ` Richard Biener 2024-07-02 9:46 ` Alex Coplan 2024-07-02 10:17 ` Alex Coplan 2024-07-02 11:41 ` Richard Biener 2024-07-02 13:36 ` Alex Coplan 2024-07-02 20:00 ` Richard Sandiford 2024-07-02 7:56 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).