[PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Tamar Christina]

[-- 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;

[PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932]

[Tamar Christina]

[-- 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.  */

RE: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Tamar Christina]

> -----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;
> 
> 
> 
> 
> --

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Richard Biener]

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)

Re: [PATCH 2/2]middle-end: replace constant_multiple_of with aff_combination_constant_multiple_p [PR114932]

[Richard Biener]

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)

RE: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Richard Biener]

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)

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Alex Coplan]

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)

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Alex Coplan]

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)

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Richard Biener]

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)

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Alex Coplan]

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)

Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]

[Richard Sandiford]

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