[1/2] middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gcc_build--master-arm |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-arm |
success
|
Test passed
|
| linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 |
success
|
Build passed
|
| linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 |
success
|
Test passed
|
Commit Message
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.
---
--
Comments
> -----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;
>
>
>
>
> --
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;
>
>
>
>
>
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;
> >
> >
> >
> >
> > --
>
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)
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)
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)
>
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)
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
@@ -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;