From: Tamar Christina <Tamar.Christina@arm.com>
To: Andrew MacLeod <amacleod@redhat.com>,
Richard Biener <rguenther@suse.de>,
Richard Sandiford <Richard.Sandiford@arm.com>
Cc: Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org>,
nd <nd@arm.com>, "jlaw@ventanamicro.com" <jlaw@ventanamicro.com>
Subject: RE: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583]
Date: Thu, 23 Feb 2023 16:56:31 +0000 [thread overview]
Message-ID: <VI1PR08MB532523FE9E19CD1C5E3D1040FFAB9@VI1PR08MB5325.eurprd08.prod.outlook.com> (raw)
In-Reply-To: <46f59672-9e6b-81a0-fccc-a4aa0c9c31b2@redhat.com>
> -----Original Message-----
> From: Andrew MacLeod <amacleod@redhat.com>
> Sent: Thursday, February 23, 2023 4:40 PM
> To: Tamar Christina <Tamar.Christina@arm.com>; Richard Biener
> <rguenther@suse.de>; Richard Sandiford <Richard.Sandiford@arm.com>
> Cc: Tamar Christina via Gcc-patches <gcc-patches@gcc.gnu.org>; nd
> <nd@arm.com>; jlaw@ventanamicro.com
> Subject: Re: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask
> by using new optabs [PR108583]
>
>
> On 2/23/23 03:36, Tamar Christina wrote:
> > Hi Andrew,
> >
> >>>> Oh yeah, and in case you haven't figured it out on your own, you'll
> >>>> have to remove WIDEN_MULT_EXPR from the range-ops init table.
> >>>> This non-standard mechanism only gets checked if there is no
> >>>> standard range-op table entry for the tree code :-P
> >>>>
> >>> Hmm it looks like it'll work, but it keeps segfaulting in:
> >>>
> >>> bool
> >>> range_op_handler::fold_range (vrange &r, tree type,
> >>> const vrange &lh,
> >>> const vrange &rh,
> >>> relation_trio rel) const
> >>> {
> >>> gcc_checking_assert (m_valid);
> >>> if (m_int)
> >>> return m_int->fold_range (as_a <irange> (r), type,
> >>> as_a <irange> (lh),
> >>> as_a <irange> (rh), rel);
> >>>
> >>> while trying to call fold_range.
> >>>
> >>> But m_int is set to the right instance. Probably something I'm
> >>> missing, I'll double check it all.
> >>>
> >> Hmm. whats your class operator_widen_mult* look like? what are you
> >> inheriting from? Send me your patch and I'll have a look if you
> >> want. this is somewhat new territory :-)
> > I've attached the patch, and my testcase is:
> >
> > int decMultiplyOp_zacc, decMultiplyOp_iacc; int *decMultiplyOp_lp;
> > void decMultiplyOp() {
> > decMultiplyOp_lp = &decMultiplyOp_zacc;
> > for (; decMultiplyOp_lp < &decMultiplyOp_zacc + decMultiplyOp_iacc;
> > decMultiplyOp_lp++)
> > *decMultiplyOp_lp = 0;
> > }
> >
> > And compiling with aarch64-none-elf-gcc -O2 zero.c -S -o -
> > -Werror=stringop-overflow
> >
> > Also to explain a bit on why we're only seeing this now:
> >
> > The original sequence for most of the pipeline is based on a cast and
> > multiplication
> >
> > # RANGE [irange] long unsigned int [0,
> 2147483647][18446744071562067968, +INF]
> > _14 = (long unsigned intD.11) decMultiplyOp_iacc.2_13;
> > # RANGE [irange] long unsigned int [0,
> 8589934588][18446744065119617024, 18446744073709551612]
> NONZERO 0xfffffffffffffffc
> > _15 = _14 * 4;
> >
> > But things like widening multiply are quite common, so some ISAs have it on
> scalars as well, not just vectors.
> > So there's a pass widening_mul that runs late for these targets. This
> > replaces the above with
> >
> > # RANGE [irange] long unsigned int [0,
> 8589934588][18446744065119617024, 18446744073709551612]
> NONZERO 0xfffffffffffffffc
> > _15 = decMultiplyOp_iacc.2_13 w* 4;
> >
> > And copies over the final range from the original expression.
> >
> > After that there are passes like the warning passes that try to requery ranged
> to see if any optimization has changed them.
> > Before my attempt to support *w this would just return VARYING and it
> would only use the old range.
> >
> > Now however, without taking care to sign extend when appropriate the
> > MIN range changes from a negative value to a large positive one when
> > we increase the precision. So passes that re-query late get the wrong range.
> That's why for instance in this case we get an incorrect warning generated.
> >
> > Thanks for the help!
> >
> > Tamar
> >
> >> I cant imagine it being a linkage thing between the 2 files since the
> >> operator is defined in another file and the address taken in this one?
> >> that should work, but strange that cant make the call...
> >>
> >> Andrew
>
> It is some sort of linkage/vtable thing. The fix.diff patch applied on top of
> what you have will fix the fold issue. This'll do for now until I formalize how this
> is going to work goign forward.
Ah, I did see gdb warning about the vtable 😊
>
> Inheriting from operator_mult is also going to be hazardous because it also
> has an op1_range and op2_range... you should at least define those and
> return VARYING to avoid other issues. Same thing applies to widen_plus I
> think, and it has relation processing and other things as well. Your widen
> operands are not what those classes expect, so I think you probably just want
> a fresh range operator.
>
> It also looks like the mult operation is sign/zero extending both upper bounds,
> and neither lower bound.. I think that should be the LH upper and lower
> bound?
Ah yes, that was a typo.
>
> I've attached a second patch (newversion.patch) which incorporates my fix,
> the fix to the sign of only op1's bounds, as well as a simplification of the
> classes to not inherit from operator_mult/plus.. I think this still does what you
> want? and it wont get you into unexpected trouble later :-)
>
> let me know if this is still doing what you are expecting...
Yes it was! And works perfectly. I think I'll need the same for widen_plus, so I'll
make those changes and do full regression run and submit the finished patch.
Thanks for all the help!
Cheers,
Tamar
>
> Andrew
next prev parent reply other threads:[~2023-02-23 16:56 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-09 17:16 Tamar Christina
2023-02-09 17:22 ` [PATCH 2/2]AArch64 Update div-bitmask to implement new optab instead of target hook [PR108583] Tamar Christina
2023-02-10 10:35 ` Tamar Christina
2023-02-10 14:10 ` Richard Sandiford
2023-02-10 10:34 ` [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583] Tamar Christina
2023-02-10 13:13 ` Richard Biener
2023-02-10 13:36 ` Richard Sandiford
2023-02-10 13:52 ` Richard Biener
2023-02-10 14:13 ` Tamar Christina
2023-02-10 14:30 ` Richard Sandiford
2023-02-10 14:54 ` Tamar Christina
2023-02-27 11:09 ` Tamar Christina
2023-02-27 12:11 ` Richard Sandiford
2023-02-27 12:14 ` Tamar Christina
2023-02-27 21:33 ` Richard Sandiford
2023-02-27 22:10 ` Tamar Christina
2023-02-28 11:08 ` Richard Sandiford
2023-02-28 11:12 ` Tamar Christina
2023-02-28 12:03 ` Richard Sandiford
2023-03-01 11:30 ` Richard Biener
2023-02-10 15:56 ` Richard Sandiford
2023-02-10 16:09 ` Tamar Christina
2023-02-10 16:25 ` Richard Sandiford
2023-02-10 16:33 ` Tamar Christina
2023-02-10 16:57 ` Richard Sandiford
2023-02-10 17:01 ` Richard Sandiford
2023-02-10 17:14 ` Tamar Christina
2023-02-10 18:12 ` Richard Sandiford
2023-02-10 18:34 ` Richard Biener
2023-02-10 20:58 ` Andrew MacLeod
2023-02-13 9:54 ` Tamar Christina
2023-02-15 12:51 ` Tamar Christina
2023-02-15 16:05 ` Andrew MacLeod
2023-02-15 17:13 ` Tamar Christina
2023-02-15 17:50 ` Andrew MacLeod
2023-02-15 18:42 ` Andrew MacLeod
2023-02-22 12:51 ` Tamar Christina
2023-02-22 16:41 ` Andrew MacLeod
2023-02-22 18:03 ` Tamar Christina
2023-02-22 18:33 ` Andrew MacLeod
2023-02-23 8:36 ` Tamar Christina
2023-02-23 16:39 ` Andrew MacLeod
2023-02-23 16:56 ` Tamar Christina [this message]
2023-03-01 16:57 ` Andrew Carlotti
2023-03-01 18:16 ` Tamar Christina
2023-02-22 13:06 ` Tamar Christina
2023-02-22 15:19 ` Andrew MacLeod
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=VI1PR08MB532523FE9E19CD1C5E3D1040FFAB9@VI1PR08MB5325.eurprd08.prod.outlook.com \
--to=tamar.christina@arm.com \
--cc=Richard.Sandiford@arm.com \
--cc=amacleod@redhat.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).