From: Andrew MacLeod <amacleod@redhat.com>
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" <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 11:39:51 -0500 [thread overview]
Message-ID: <46f59672-9e6b-81a0-fccc-a4aa0c9c31b2@redhat.com> (raw)
In-Reply-To: <VI1PR08MB5325352F0098BDBA8CF73A2BFFAB9@VI1PR08MB5325.eurprd08.prod.outlook.com>
[-- Attachment #1: Type: text/plain, Size: 4346 bytes --]
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.
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?
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...
Andrew
[-- Attachment #2: fix.diff --]
[-- Type: text/x-patch, Size: 1825 bytes --]
diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc
index 81089876d30..824e0338f34 100644
--- a/gcc/gimple-range-op.cc
+++ b/gcc/gimple-range-op.cc
@@ -777,8 +777,6 @@ gimple_range_op_handler::maybe_non_standard ()
{
case WIDEN_MULT_EXPR:
{
- extern class range_operator &op_widen_mult_signed;
- extern class range_operator &op_widen_mult_unsigned;
m_valid = true;
m_op1 = gimple_assign_rhs1 (m_stmt);
m_op2 = gimple_assign_rhs2 (m_stmt);
@@ -788,9 +786,9 @@ gimple_range_op_handler::maybe_non_standard ()
std::swap (m_op1, m_op2);
if (signed1 || signed2)
- m_int = &op_widen_mult_signed;
+ m_int = ptr_op_widen_mult_signed;
else
- m_int = &op_widen_mult_unsigned;
+ m_int = ptr_op_widen_mult_unsigned;
break;
}
default:
diff --git a/gcc/range-op.cc b/gcc/range-op.cc
index c15bd83b077..bace915b256 100644
--- a/gcc/range-op.cc
+++ b/gcc/range-op.cc
@@ -2072,6 +2072,7 @@ public:
const wide_int &w0, const wide_int &w1)
const;
} op_widen_mult_signed;
+range_operator *ptr_op_widen_mult_signed = &op_widen_mult_signed;
void
operator_widen_mult_signed::wi_fold (irange &r, tree type,
@@ -2119,6 +2120,7 @@ public:
const wide_int &w0, const wide_int &w1)
const;
} op_widen_mult_unsigned;
+range_operator *ptr_op_widen_mult_unsigned = &op_widen_mult_unsigned;
void
operator_widen_mult_unsigned::wi_fold (irange &r, tree type,
diff --git a/gcc/range-op.h b/gcc/range-op.h
index f00b747f08a..5fe463234ae 100644
--- a/gcc/range-op.h
+++ b/gcc/range-op.h
@@ -311,4 +311,6 @@ private:
// This holds the range op table for floating point operations.
extern floating_op_table *floating_tree_table;
+extern range_operator *ptr_op_widen_mult_signed;
+extern range_operator *ptr_op_widen_mult_unsigned;
#endif // GCC_RANGE_OP_H
[-- Attachment #3: newversion.patch --]
[-- Type: text/x-patch, Size: 6125 bytes --]
diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc
index d9dfdc56939..824e0338f34 100644
--- a/gcc/gimple-range-op.cc
+++ b/gcc/gimple-range-op.cc
@@ -179,6 +179,8 @@ gimple_range_op_handler::gimple_range_op_handler (gimple *s)
// statements.
if (is_a <gcall *> (m_stmt))
maybe_builtin_call ();
+ else
+ maybe_non_standard ();
}
// Calculate what we can determine of the range of this unary
@@ -764,6 +766,36 @@ public:
}
} op_cfn_parity;
+// Set up a gimple_range_op_handler for any nonstandard function which can be
+// supported via range-ops.
+
+void
+gimple_range_op_handler::maybe_non_standard ()
+{
+ if (gimple_code (m_stmt) == GIMPLE_ASSIGN)
+ switch (gimple_assign_rhs_code (m_stmt))
+ {
+ case WIDEN_MULT_EXPR:
+ {
+ m_valid = true;
+ m_op1 = gimple_assign_rhs1 (m_stmt);
+ m_op2 = gimple_assign_rhs2 (m_stmt);
+ bool signed1 = TYPE_SIGN (TREE_TYPE (m_op1)) == SIGNED;
+ bool signed2 = TYPE_SIGN (TREE_TYPE (m_op2)) == SIGNED;
+ if (signed2 && !signed1)
+ std::swap (m_op1, m_op2);
+
+ if (signed1 || signed2)
+ m_int = ptr_op_widen_mult_signed;
+ else
+ m_int = ptr_op_widen_mult_unsigned;
+ break;
+ }
+ default:
+ break;
+ }
+}
+
// Set up a gimple_range_op_handler for any built in function which can be
// supported via range-ops.
diff --git a/gcc/gimple-range-op.h b/gcc/gimple-range-op.h
index 743b858126e..1bf63c5ce6f 100644
--- a/gcc/gimple-range-op.h
+++ b/gcc/gimple-range-op.h
@@ -41,6 +41,7 @@ public:
relation_trio = TRIO_VARYING);
private:
void maybe_builtin_call ();
+ void maybe_non_standard ();
gimple *m_stmt;
tree m_op1, m_op2;
};
diff --git a/gcc/range-op.cc b/gcc/range-op.cc
index 5c67bce6d3a..7cd19a92d00 100644
--- a/gcc/range-op.cc
+++ b/gcc/range-op.cc
@@ -1556,6 +1556,34 @@ operator_plus::op2_range (irange &r, tree type,
return op1_range (r, type, lhs, op1, rel.swap_op1_op2 ());
}
+class operator_widen_plus : public range_operator
+{
+public:
+ virtual void wi_fold (irange &r, tree type,
+ const wide_int &lh_lb,
+ const wide_int &lh_ub,
+ const wide_int &rh_lb,
+ const wide_int &rh_ub) const;
+} op_widen_plus;
+
+void
+operator_widen_plus::wi_fold (irange &r, tree type,
+ const wide_int &lh_lb, const wide_int &lh_ub,
+ const wide_int &rh_lb, const wide_int &rh_ub) const
+{
+ wi::overflow_type ov_lb, ov_ub;
+ signop s = TYPE_SIGN (type);
+
+ wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, s);
+ wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
+ wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, s);
+ wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
+
+ wide_int new_lb = wi::add (lh_wlb, rh_wlb, s, &ov_lb);
+ wide_int new_ub = wi::add (lh_wub, rh_wub, s, &ov_ub);
+
+ r = int_range<2> (type, new_lb, new_ub);
+}
class operator_minus : public range_operator
{
@@ -2031,6 +2059,70 @@ operator_mult::wi_fold (irange &r, tree type,
}
}
+class operator_widen_mult_signed : public range_operator
+{
+public:
+ virtual void wi_fold (irange &r, tree type,
+ const wide_int &lh_lb,
+ const wide_int &lh_ub,
+ const wide_int &rh_lb,
+ const wide_int &rh_ub)
+ const;
+} op_widen_mult_signed;
+range_operator *ptr_op_widen_mult_signed = &op_widen_mult_signed;
+
+void
+operator_widen_mult_signed::wi_fold (irange &r, tree type,
+ const wide_int &lh_lb,
+ const wide_int &lh_ub,
+ const wide_int &rh_lb,
+ const wide_int &rh_ub) const
+{
+ signop s = TYPE_SIGN (type);
+
+ wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, SIGNED);
+ wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, SIGNED);
+ wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
+ wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
+
+ /* We don't expect a widening multiplication to be able to overflow but range
+ calculations for multiplications are complicated. After widening the
+ operands lets call the base class. */
+ return op_mult.wi_fold (r, type, lh_wlb, lh_wub, rh_wlb, rh_wub);
+}
+
+
+class operator_widen_mult_unsigned : public range_operator
+{
+public:
+ virtual void wi_fold (irange &r, tree type,
+ const wide_int &lh_lb,
+ const wide_int &lh_ub,
+ const wide_int &rh_lb,
+ const wide_int &rh_ub)
+ const;
+} op_widen_mult_unsigned;
+range_operator *ptr_op_widen_mult_unsigned = &op_widen_mult_unsigned;
+
+void
+operator_widen_mult_unsigned::wi_fold (irange &r, tree type,
+ const wide_int &lh_lb,
+ const wide_int &lh_ub,
+ const wide_int &rh_lb,
+ const wide_int &rh_ub) const
+{
+ signop s = TYPE_SIGN (type);
+
+ wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, UNSIGNED);
+ wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, UNSIGNED);
+ wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
+ wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
+
+ /* We don't expect a widening multiplication to be able to overflow but range
+ calculations for multiplications are complicated. After widening the
+ operands lets call the base class. */
+ return op_mult.wi_fold (r, type, lh_wlb, lh_wub, rh_wlb, rh_wub);
+}
class operator_div : public cross_product_operator
{
@@ -4473,6 +4565,7 @@ integral_table::integral_table ()
set (GT_EXPR, op_gt);
set (GE_EXPR, op_ge);
set (PLUS_EXPR, op_plus);
+ set (WIDEN_PLUS_EXPR, op_widen_plus);
set (MINUS_EXPR, op_minus);
set (MIN_EXPR, op_min);
set (MAX_EXPR, op_max);
diff --git a/gcc/range-op.h b/gcc/range-op.h
index f00b747f08a..5fe463234ae 100644
--- a/gcc/range-op.h
+++ b/gcc/range-op.h
@@ -311,4 +311,6 @@ private:
// This holds the range op table for floating point operations.
extern floating_op_table *floating_tree_table;
+extern range_operator *ptr_op_widen_mult_signed;
+extern range_operator *ptr_op_widen_mult_unsigned;
#endif // GCC_RANGE_OP_H
next prev parent reply other threads:[~2023-02-23 16:39 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 [this message]
2023-02-23 16:56 ` Tamar Christina
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=46f59672-9e6b-81a0-fccc-a4aa0c9c31b2@redhat.com \
--to=amacleod@redhat.com \
--cc=Richard.Sandiford@arm.com \
--cc=Tamar.Christina@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).