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

[Alex Coplan <alex.coplan@arm.com>]

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)