From: Richard Sandiford <richard.sandiford@arm.com>
To: Alex Coplan <alex.coplan@arm.com>
Cc: Richard Biener <rguenther@suse.de>,
Tamar Christina <Tamar.Christina@arm.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>,
"jlaw@ventanamicro.com" <jlaw@ventanamicro.com>
Subject: Re: [PATCH 1/2]middle-end: fix wide_int_constant_multiple_p when VAL and DIV are 0. [PR114932]
Date: Tue, 02 Jul 2024 21:00:46 +0100 [thread overview]
Message-ID: <mptplrvftsh.fsf@arm.com> (raw)
In-Reply-To: <ZoQCVi52rbCZGOLq@arm.com> (Alex Coplan's message of "Tue, 2 Jul 2024 14:36:22 +0100")
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
next prev parent reply other threads:[~2024-07-02 20:00 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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
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 [this message]
2024-07-02 7:56 ` Richard Biener
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=mptplrvftsh.fsf@arm.com \
--to=richard.sandiford@arm.com \
--cc=Tamar.Christina@arm.com \
--cc=alex.coplan@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jlaw@ventanamicro.com \
--cc=nd@arm.com \
--cc=rguenther@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).