* [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement
@ 2023-08-11 9:15 Andrew Pinski
2023-08-11 9:15 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
2023-08-11 9:50 ` [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Richard Biener
0 siblings, 2 replies; 13+ messages in thread
From: Andrew Pinski @ 2023-08-11 9:15 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
So with my next VRP patch, VRP causes:
```
# c$_M_value_18 = PHI <-1(3), 0(2), 1(4)>
_11 = (unsigned int) c$_M_value_18;
_16 = _11 <= 1;
```
To be changed to:
```
# c$_M_value_18 = PHI <-1(3), 0(2), 1(4)>
_11 = (unsigned int) c$_M_value_18;
_16 = _11 != 4294967295;
```
So let's add support for the above.
A few changes was needed, first to change
the range check of the rhs of the comparison to possibly
integer_all_onesp also.
The next is to add support for the cast and EQ/NE case.
Note on the testcases pr110984-1.c is basically pr94589-2.c but
with what the C++ code is doing with the signed char type;
pr110984-2.c is pr110984-1.c with the cast added to give an
explicit testcase to test against.
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
Thanks,
Andrew Pinski
PR tree-optimization/110984
gcc/ChangeLog:
* tree-ssa-phiopt.cc (spaceship_replacement): Add support for
NE/EQ for the cast case.
gcc/testsuite/ChangeLog:
* gcc.dg/pr110984-1.c: New test.
* gcc.dg/pr110984-2.c: New test.
---
gcc/testsuite/gcc.dg/pr110984-1.c | 37 +++++++++++++++++++++++++++++++
gcc/testsuite/gcc.dg/pr110984-2.c | 21 ++++++++++++++++++
gcc/tree-ssa-phiopt.cc | 19 +++++++++++++---
3 files changed, 74 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/pr110984-1.c
create mode 100644 gcc/testsuite/gcc.dg/pr110984-2.c
diff --git a/gcc/testsuite/gcc.dg/pr110984-1.c b/gcc/testsuite/gcc.dg/pr110984-1.c
new file mode 100644
index 00000000000..85b19eb8279
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr110984-1.c
@@ -0,0 +1,37 @@
+/* PR tree-optimization/110984 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -g0 -ffast-math -fdump-tree-optimized" } */
+/* { dg-final { scan-tree-dump-times "\[ij]_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) \[ij]_\[0-9]+\\(D\\)" 14 "optimized" } } */
+/* { dg-final { scan-tree-dump-times "i_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) 5\\.0" 14 "optimized" } } */
+
+/* This is similar to pr94589-2.c except use signed char as the type for the [-1,2] case */
+
+#define A __attribute__((noipa))
+A int f1 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == 0; }
+A int f2 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != 0; }
+A int f3 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c > 0; }
+A int f4 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c < 0; }
+A int f5 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c >= 0; }
+A int f6 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c <= 0; }
+A int f7 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == -1; }
+A int f8 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != -1; }
+A int f9 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c > -1; }
+A int f10 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c <= -1; }
+A int f11 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == 1; }
+A int f12 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != 1; }
+A int f13 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c < 1; }
+A int f14 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c >= 1; }
+A int f15 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == 0; }
+A int f16 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != 0; }
+A int f17 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c > 0; }
+A int f18 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c < 0; }
+A int f19 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c >= 0; }
+A int f20 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c <= 0; }
+A int f21 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == -1; }
+A int f22 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != -1; }
+A int f23 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c > -1; }
+A int f24 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c <= -1; }
+A int f25 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == 1; }
+A int f26 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != 1; }
+A int f27 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c < 1; }
+A int f28 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c >= 1; }
diff --git a/gcc/testsuite/gcc.dg/pr110984-2.c b/gcc/testsuite/gcc.dg/pr110984-2.c
new file mode 100644
index 00000000000..cddce045745
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr110984-2.c
@@ -0,0 +1,21 @@
+/* PR tree-optimization/110984 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -g0 -ffast-math -fdump-tree-optimized" } */
+/* { dg-final { scan-tree-dump-times "\[ij]_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) \[ij]_\[0-9]+\\(D\\)" 6 "optimized" } } */
+/* { dg-final { scan-tree-dump-times "i_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) 5\\.0" 6 "optimized" } } */
+
+/* This is similar to pr94589-2.c except use signed char and use unsigned types to check against the variable, which means removing the non !=/== comparisons. */
+
+#define A __attribute__((noipa))
+A int f1 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == 0; }
+A int f2 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != 0; }
+A int f7 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == -1; }
+A int f8 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != -1; }
+A int f11 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == 1; }
+A int f12 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != 1; }
+A int f15 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == 0; }
+A int f16 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != 0; }
+A int f21 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == -1; }
+A int f22 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != -1; }
+A int f25 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == 1; }
+A int f26 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != 1; }
diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
index 485662dfcc7..5f24d62c49a 100644
--- a/gcc/tree-ssa-phiopt.cc
+++ b/gcc/tree-ssa-phiopt.cc
@@ -2346,7 +2346,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
}
if (lhs != (orig_use_lhs ? orig_use_lhs : phires)
|| !tree_fits_shwi_p (rhs)
- || !IN_RANGE (tree_to_shwi (rhs), -1, 1))
+ || !(IN_RANGE (tree_to_shwi (rhs), 0, 1)
+ || integer_all_onesp (rhs)))
return false;
if (is_cast)
@@ -2356,9 +2357,20 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
/* As for -ffast-math we assume the 2 return to be
impossible, canonicalize (unsigned) res <= 1U or
(unsigned) res < 2U into res >= 0 and (unsigned) res > 1U
- or (unsigned) res >= 2U as res < 0. */
+ or (unsigned) res >= 2U as res < 0.
+ Sometimes we get (unsigned)res != N. Support those cases too. */
switch (cmp)
{
+ case NE_EXPR:
+ case EQ_EXPR:
+ {
+ tree newrhs = fold_convert (TREE_TYPE (phires), rhs);
+ tree tmp = fold_convert (TREE_TYPE (rhs), newrhs);
+ if (!tree_int_cst_equal (rhs, tmp))
+ return false;
+ rhs = newrhs;
+ break;
+ }
case LE_EXPR:
if (!integer_onep (rhs))
return false;
@@ -2382,7 +2394,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
default:
return false;
}
- rhs = build_zero_cst (TREE_TYPE (phires));
+ if (cmp != EQ_EXPR && cmp != NE_EXPR)
+ rhs = build_zero_cst (TREE_TYPE (phires));
}
else if (orig_use_lhs)
{
--
2.31.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 9:15 [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Andrew Pinski
@ 2023-08-11 9:15 ` Andrew Pinski
2023-08-11 9:51 ` Richard Biener
2023-08-11 9:50 ` [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Richard Biener
1 sibling, 1 reply; 13+ messages in thread
From: Andrew Pinski @ 2023-08-11 9:15 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
So it turns out there was a simplier way of starting to
improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
That was rewrite test_for_singularity to use range_op_handler
and Value_Range.
This patch implements that and
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
gcc/ChangeLog:
* vr-values.cc (test_for_singularity): Add edge argument
and rewrite using range_op_handler.
(simplify_compare_using_range_pairs): Use Value_Range
instead of value_range and update test_for_singularity call.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/vrp124.c: New test.
* gcc.dg/tree-ssa/vrp125.c: New test.
---
gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++
gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++
gcc/vr-values.cc | 91 ++++++++------------------
3 files changed, 114 insertions(+), 65 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
new file mode 100644
index 00000000000..6ccbda35d1b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+/* Should be optimized to a == -100 */
+int g(int a)
+{
+ if (a == -100 || a >= 0)
+ ;
+ else
+ return 0;
+ return a < 0;
+}
+
+/* Should optimize to a == 0 */
+int f(int a)
+{
+ if (a == 0 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 50;
+}
+
+/* Should be optimized to a == 0. */
+int f2(int a)
+{
+ if (a == 0 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 100;
+}
+
+/* Should optimize to a == 100 */
+int f1(int a)
+{
+ if (a < 0 || a == 100)
+ ;
+ else
+ return 0;
+ return a > 50;
+}
+
+/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
new file mode 100644
index 00000000000..f6c2f8e35f1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+/* Should be optimized to a == -100 */
+int g(int a)
+{
+ if (a == -100 || a == -50 || a >= 0)
+ ;
+ else
+ return 0;
+ return a < -50;
+}
+
+/* Should optimize to a == 0 */
+int f(int a)
+{
+ if (a == 0 || a == 50 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 50;
+}
+
+/* Should be optimized to a == 0. */
+int f2(int a)
+{
+ if (a == 0 || a == 50 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 25;
+}
+
+/* Should optimize to a == 100 */
+int f1(int a)
+{
+ if (a < 0 || a == 50 || a == 100)
+ ;
+ else
+ return 0;
+ return a > 50;
+}
+
+/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
index a4fddd62841..7004b0224bd 100644
--- a/gcc/vr-values.cc
+++ b/gcc/vr-values.cc
@@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
a known value range VR.
If there is one and only one value which will satisfy the
- conditional, then return that value. Else return NULL.
-
- If signed overflow must be undefined for the value to satisfy
- the conditional, then set *STRICT_OVERFLOW_P to true. */
+ conditional on the EDGE, then return that value.
+ Else return NULL. */
static tree
test_for_singularity (enum tree_code cond_code, tree op0,
- tree op1, const value_range *vr)
+ tree op1, Value_Range vr, bool edge)
{
- tree min = NULL;
- tree max = NULL;
-
- /* Extract minimum/maximum values which satisfy the conditional as it was
- written. */
- if (cond_code == LE_EXPR || cond_code == LT_EXPR)
+ /* This is already a singularity. */
+ if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
+ return NULL;
+ auto range_op = range_op_handler (cond_code);
+ int_range<2> op1_range (TREE_TYPE (op0));
+ wide_int w = wi::to_wide (op1);
+ op1_range.set (TREE_TYPE (op1), w, w);
+ Value_Range vr1(TREE_TYPE (op0));
+ if (range_op.op1_range (vr1, TREE_TYPE (op0),
+ edge ? range_true () : range_false (),
+ op1_range))
{
- min = TYPE_MIN_VALUE (TREE_TYPE (op0));
-
- max = op1;
- if (cond_code == LT_EXPR)
- {
- tree one = build_int_cst (TREE_TYPE (op0), 1);
- max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
- /* Signal to compare_values_warnv this expr doesn't overflow. */
- if (EXPR_P (max))
- suppress_warning (max, OPT_Woverflow);
- }
- }
- else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
- {
- max = TYPE_MAX_VALUE (TREE_TYPE (op0));
-
- min = op1;
- if (cond_code == GT_EXPR)
- {
- tree one = build_int_cst (TREE_TYPE (op0), 1);
- min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
- /* Signal to compare_values_warnv this expr doesn't overflow. */
- if (EXPR_P (min))
- suppress_warning (min, OPT_Woverflow);
- }
- }
-
- /* Now refine the minimum and maximum values using any
- value range information we have for op0. */
- if (min && max)
- {
- tree type = TREE_TYPE (op0);
- tree tmin = wide_int_to_tree (type, vr->lower_bound ());
- tree tmax = wide_int_to_tree (type, vr->upper_bound ());
- if (compare_values (tmin, min) == 1)
- min = tmin;
- if (compare_values (tmax, max) == -1)
- max = tmax;
-
- /* If the new min/max values have converged to a single value,
- then there is only one value which can satisfy the condition,
- return that value. */
- if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
- return min;
+ vr.intersect (vr1);
+ tree newop1;
+ /* If the updated range is just a singleton, then we can just do a comparison */
+ if (vr.singleton_p (&newop1))
+ return newop1;
}
return NULL;
}
@@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
&& cond_code != EQ_EXPR
&& TREE_CODE (op0) == SSA_NAME
&& INTEGRAL_TYPE_P (TREE_TYPE (op0))
- && is_gimple_min_invariant (op1))
+ && TREE_CODE (op1) == INTEGER_CST)
{
- value_range vr;
+ Value_Range vr (TREE_TYPE (op0));
if (!query->range_of_expr (vr, op0, stmt))
vr.set_undefined ();
@@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
able to simplify this conditional. */
if (!vr.undefined_p () && !vr.varying_p ())
{
- tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
+ tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
+ true);
if (new_tree)
{
cond_code = EQ_EXPR;
op1 = new_tree;
happened = true;
}
-
- /* Try again after inverting the condition. We only deal
- with integral types here, so no need to worry about
- issues with inverting FP comparisons. */
- new_tree = test_for_singularity
- (invert_tree_comparison (cond_code, false),
- op0, op1, &vr);
+ /* Try again after inverting the condition. */
+ new_tree = test_for_singularity (cond_code, op0, op1, vr,
+ false);
if (new_tree)
{
cond_code = NE_EXPR;
--
2.31.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 9:15 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
@ 2023-08-11 9:51 ` Richard Biener
2023-08-11 14:00 ` Jeff Law
2023-08-11 15:07 ` Andrew MacLeod
0 siblings, 2 replies; 13+ messages in thread
From: Richard Biener @ 2023-08-11 9:51 UTC (permalink / raw)
To: Andrew Pinski, Andrew MacLeod, Aldy Hernandez; +Cc: gcc-patches
On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> So it turns out there was a simplier way of starting to
> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> That was rewrite test_for_singularity to use range_op_handler
> and Value_Range.
>
> This patch implements that and
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
I'm hoping Andrew/Aldy can have a look here.
Richard.
> gcc/ChangeLog:
>
> * vr-values.cc (test_for_singularity): Add edge argument
> and rewrite using range_op_handler.
> (simplify_compare_using_range_pairs): Use Value_Range
> instead of value_range and update test_for_singularity call.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/vrp124.c: New test.
> * gcc.dg/tree-ssa/vrp125.c: New test.
> ---
> gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++
> gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++
> gcc/vr-values.cc | 91 ++++++++------------------
> 3 files changed, 114 insertions(+), 65 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> new file mode 100644
> index 00000000000..6ccbda35d1b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> @@ -0,0 +1,44 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +/* Should be optimized to a == -100 */
> +int g(int a)
> +{
> + if (a == -100 || a >= 0)
> + ;
> + else
> + return 0;
> + return a < 0;
> +}
> +
> +/* Should optimize to a == 0 */
> +int f(int a)
> +{
> + if (a == 0 || a > 100)
> + ;
> + else
> + return 0;
> + return a < 50;
> +}
> +
> +/* Should be optimized to a == 0. */
> +int f2(int a)
> +{
> + if (a == 0 || a > 100)
> + ;
> + else
> + return 0;
> + return a < 100;
> +}
> +
> +/* Should optimize to a == 100 */
> +int f1(int a)
> +{
> + if (a < 0 || a == 100)
> + ;
> + else
> + return 0;
> + return a > 50;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> new file mode 100644
> index 00000000000..f6c2f8e35f1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> @@ -0,0 +1,44 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> +
> +/* Should be optimized to a == -100 */
> +int g(int a)
> +{
> + if (a == -100 || a == -50 || a >= 0)
> + ;
> + else
> + return 0;
> + return a < -50;
> +}
> +
> +/* Should optimize to a == 0 */
> +int f(int a)
> +{
> + if (a == 0 || a == 50 || a > 100)
> + ;
> + else
> + return 0;
> + return a < 50;
> +}
> +
> +/* Should be optimized to a == 0. */
> +int f2(int a)
> +{
> + if (a == 0 || a == 50 || a > 100)
> + ;
> + else
> + return 0;
> + return a < 25;
> +}
> +
> +/* Should optimize to a == 100 */
> +int f1(int a)
> +{
> + if (a < 0 || a == 50 || a == 100)
> + ;
> + else
> + return 0;
> + return a > 50;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> index a4fddd62841..7004b0224bd 100644
> --- a/gcc/vr-values.cc
> +++ b/gcc/vr-values.cc
> @@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> a known value range VR.
>
> If there is one and only one value which will satisfy the
> - conditional, then return that value. Else return NULL.
> -
> - If signed overflow must be undefined for the value to satisfy
> - the conditional, then set *STRICT_OVERFLOW_P to true. */
> + conditional on the EDGE, then return that value.
> + Else return NULL. */
>
> static tree
> test_for_singularity (enum tree_code cond_code, tree op0,
> - tree op1, const value_range *vr)
> + tree op1, Value_Range vr, bool edge)
> {
> - tree min = NULL;
> - tree max = NULL;
> -
> - /* Extract minimum/maximum values which satisfy the conditional as it was
> - written. */
> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> + /* This is already a singularity. */
> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> + return NULL;
> + auto range_op = range_op_handler (cond_code);
> + int_range<2> op1_range (TREE_TYPE (op0));
> + wide_int w = wi::to_wide (op1);
> + op1_range.set (TREE_TYPE (op1), w, w);
> + Value_Range vr1(TREE_TYPE (op0));
> + if (range_op.op1_range (vr1, TREE_TYPE (op0),
> + edge ? range_true () : range_false (),
> + op1_range))
> {
> - min = TYPE_MIN_VALUE (TREE_TYPE (op0));
> -
> - max = op1;
> - if (cond_code == LT_EXPR)
> - {
> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> - max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> - if (EXPR_P (max))
> - suppress_warning (max, OPT_Woverflow);
> - }
> - }
> - else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
> - {
> - max = TYPE_MAX_VALUE (TREE_TYPE (op0));
> -
> - min = op1;
> - if (cond_code == GT_EXPR)
> - {
> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> - min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> - if (EXPR_P (min))
> - suppress_warning (min, OPT_Woverflow);
> - }
> - }
> -
> - /* Now refine the minimum and maximum values using any
> - value range information we have for op0. */
> - if (min && max)
> - {
> - tree type = TREE_TYPE (op0);
> - tree tmin = wide_int_to_tree (type, vr->lower_bound ());
> - tree tmax = wide_int_to_tree (type, vr->upper_bound ());
> - if (compare_values (tmin, min) == 1)
> - min = tmin;
> - if (compare_values (tmax, max) == -1)
> - max = tmax;
> -
> - /* If the new min/max values have converged to a single value,
> - then there is only one value which can satisfy the condition,
> - return that value. */
> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> - return min;
> + vr.intersect (vr1);
> + tree newop1;
> + /* If the updated range is just a singleton, then we can just do a comparison */
> + if (vr.singleton_p (&newop1))
> + return newop1;
> }
> return NULL;
> }
> @@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> && cond_code != EQ_EXPR
> && TREE_CODE (op0) == SSA_NAME
> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> - && is_gimple_min_invariant (op1))
> + && TREE_CODE (op1) == INTEGER_CST)
> {
> - value_range vr;
> + Value_Range vr (TREE_TYPE (op0));
>
> if (!query->range_of_expr (vr, op0, stmt))
> vr.set_undefined ();
> @@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> able to simplify this conditional. */
> if (!vr.undefined_p () && !vr.varying_p ())
> {
> - tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
> + tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
> + true);
> if (new_tree)
> {
> cond_code = EQ_EXPR;
> op1 = new_tree;
> happened = true;
> }
> -
> - /* Try again after inverting the condition. We only deal
> - with integral types here, so no need to worry about
> - issues with inverting FP comparisons. */
> - new_tree = test_for_singularity
> - (invert_tree_comparison (cond_code, false),
> - op0, op1, &vr);
> + /* Try again after inverting the condition. */
> + new_tree = test_for_singularity (cond_code, op0, op1, vr,
> + false);
> if (new_tree)
> {
> cond_code = NE_EXPR;
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 9:51 ` Richard Biener
@ 2023-08-11 14:00 ` Jeff Law
2023-08-11 15:07 ` Andrew MacLeod
1 sibling, 0 replies; 13+ messages in thread
From: Jeff Law @ 2023-08-11 14:00 UTC (permalink / raw)
To: Richard Biener, Andrew Pinski, Andrew MacLeod, Aldy Hernandez; +Cc: gcc-patches
On 8/11/23 03:51, Richard Biener via Gcc-patches wrote:
> On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> So it turns out there was a simplier way of starting to
>> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
>> That was rewrite test_for_singularity to use range_op_handler
>> and Value_Range.
>>
>> This patch implements that and
>>
>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> I'm hoping Andrew/Aldy can have a look here.
It's actually pretty simple stuff. Instead of open-coding range
identification for op1 and simplification of that range using op0's
known range (VR), instead we generate a real range for op1 and intersect
that result with the known range for op0. If the result is a singleton,
return it. Simpler and more effective in the end.
I guess the interactions with the warning subsystem are a non-issue in
the updated code since it doesn't return an expression, but the
singleton value (when in the hell did it start returning an expression,
that just seems wrong given the result is supposed to be a singleton!)
LGTM.
Jeff
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 9:51 ` Richard Biener
2023-08-11 14:00 ` Jeff Law
@ 2023-08-11 15:07 ` Andrew MacLeod
2023-08-21 21:00 ` Andrew Pinski
2023-09-01 6:40 ` Andrew Pinski
1 sibling, 2 replies; 13+ messages in thread
From: Andrew MacLeod @ 2023-08-11 15:07 UTC (permalink / raw)
To: Richard Biener, Andrew Pinski, Aldy Hernandez; +Cc: gcc-patches
On 8/11/23 05:51, Richard Biener wrote:
> On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> So it turns out there was a simplier way of starting to
>> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
>> That was rewrite test_for_singularity to use range_op_handler
>> and Value_Range.
>>
>> This patch implements that and
>>
>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> I'm hoping Andrew/Aldy can have a look here.
>
> Richard.
>
>> gcc/ChangeLog:
>>
>> * vr-values.cc (test_for_singularity): Add edge argument
>> and rewrite using range_op_handler.
>> (simplify_compare_using_range_pairs): Use Value_Range
>> instead of value_range and update test_for_singularity call.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.dg/tree-ssa/vrp124.c: New test.
>> * gcc.dg/tree-ssa/vrp125.c: New test.
>> ---
>> gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++
>> gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++
>> gcc/vr-values.cc | 91 ++++++++------------------
>> 3 files changed, 114 insertions(+), 65 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
>> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
>>
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
>> new file mode 100644
>> index 00000000000..6ccbda35d1b
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
>> @@ -0,0 +1,44 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +
>> +/* Should be optimized to a == -100 */
>> +int g(int a)
>> +{
>> + if (a == -100 || a >= 0)
>> + ;
>> + else
>> + return 0;
>> + return a < 0;
>> +}
>> +
>> +/* Should optimize to a == 0 */
>> +int f(int a)
>> +{
>> + if (a == 0 || a > 100)
>> + ;
>> + else
>> + return 0;
>> + return a < 50;
>> +}
>> +
>> +/* Should be optimized to a == 0. */
>> +int f2(int a)
>> +{
>> + if (a == 0 || a > 100)
>> + ;
>> + else
>> + return 0;
>> + return a < 100;
>> +}
>> +
>> +/* Should optimize to a == 100 */
>> +int f1(int a)
>> +{
>> + if (a < 0 || a == 100)
>> + ;
>> + else
>> + return 0;
>> + return a > 50;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
>> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
>> new file mode 100644
>> index 00000000000..f6c2f8e35f1
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
>> @@ -0,0 +1,44 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -fdump-tree-optimized" } */
>> +
>> +/* Should be optimized to a == -100 */
>> +int g(int a)
>> +{
>> + if (a == -100 || a == -50 || a >= 0)
>> + ;
>> + else
>> + return 0;
>> + return a < -50;
>> +}
>> +
>> +/* Should optimize to a == 0 */
>> +int f(int a)
>> +{
>> + if (a == 0 || a == 50 || a > 100)
>> + ;
>> + else
>> + return 0;
>> + return a < 50;
>> +}
>> +
>> +/* Should be optimized to a == 0. */
>> +int f2(int a)
>> +{
>> + if (a == 0 || a == 50 || a > 100)
>> + ;
>> + else
>> + return 0;
>> + return a < 25;
>> +}
>> +
>> +/* Should optimize to a == 100 */
>> +int f1(int a)
>> +{
>> + if (a < 0 || a == 50 || a == 100)
>> + ;
>> + else
>> + return 0;
>> + return a > 50;
>> +}
>> +
>> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
>> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
>> index a4fddd62841..7004b0224bd 100644
>> --- a/gcc/vr-values.cc
>> +++ b/gcc/vr-values.cc
>> @@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
>> a known value range VR.
>>
>> If there is one and only one value which will satisfy the
>> - conditional, then return that value. Else return NULL.
>> -
>> - If signed overflow must be undefined for the value to satisfy
>> - the conditional, then set *STRICT_OVERFLOW_P to true. */
>> + conditional on the EDGE, then return that value.
>> + Else return NULL. */
>>
>> static tree
>> test_for_singularity (enum tree_code cond_code, tree op0,
>> - tree op1, const value_range *vr)
>> + tree op1, Value_Range vr, bool edge)
VR should be a "vrange &". THis is the top level base class for all
ranges of all types/kinds, and what we usually pass values around as if
we want tohem to be any kind. If this is inetger only, we'd pass a an
'irange &'
Value_Range is the opposite. Its the sink that contains one of each kind
of range and can switch around between them as needed. You do not want
to pass that by value! The generic engine uses these so it can suppose
floats. int, pointers, whatever...
>> {
>> - tree min = NULL;
>> - tree max = NULL;
>> -
>> - /* Extract minimum/maximum values which satisfy the conditional as it was
>> - written. */
>> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
>> + /* This is already a singularity. */
>> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
>> + return NULL;
>> + auto range_op = range_op_handler (cond_code);
>> + int_range<2> op1_range (TREE_TYPE (op0));
>> + wide_int w = wi::to_wide (op1);
>> + op1_range.set (TREE_TYPE (op1), w, w);
If this is only going to work with integers, you might want to check
that somewhere or switch to irange and int_range_max..
You can make it work with any kind (if you know op1 is a constant) by
simply doing
Value_Range op1_range (TREE_TYPE (op1))
get_global_range_query->range_of_expr (op1_range, op1)
That will convert trees to a the appropriate range... THis is also true
for integer constants... but you can also just do the WI conversion like
you do.
The routine also get confusing to read because it passes in op0 and
op1, but of course ranger uses op1 and op2 nomenclature, and it looks a
bit confusing :-P I'd change the operands passed in to op1 and op2 if
we are rewriting the routine.
>> + Value_Range vr1(TREE_TYPE (op0));
>> + if (range_op.op1_range (vr1, TREE_TYPE (op0),
>> + edge ? range_true () : range_false (),
>> + op1_range))
IF you decide to stick with integers, then you can just make vr1 an
int_range_max vr1;
You don't need the type in the declaration if you know its an irange.
Value_Range needs a type because it needs to know if its an integr or a
floating point (or whatever) that it is going ot be used as.
>> {
>> - min = TYPE_MIN_VALUE (TREE_TYPE (op0));
>> -
>> - max = op1;
>> - if (cond_code == LT_EXPR)
>> - {
>> - tree one = build_int_cst (TREE_TYPE (op0), 1);
>> - max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
>> - /* Signal to compare_values_warnv this expr doesn't overflow. */
>> - if (EXPR_P (max))
>> - suppress_warning (max, OPT_Woverflow);
>> - }
>> - }
>> - else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
>> - {
>> - max = TYPE_MAX_VALUE (TREE_TYPE (op0));
>> -
>> - min = op1;
>> - if (cond_code == GT_EXPR)
>> - {
>> - tree one = build_int_cst (TREE_TYPE (op0), 1);
>> - min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
>> - /* Signal to compare_values_warnv this expr doesn't overflow. */
>> - if (EXPR_P (min))
>> - suppress_warning (min, OPT_Woverflow);
>> - }
>> - }
>> -
>> - /* Now refine the minimum and maximum values using any
>> - value range information we have for op0. */
>> - if (min && max)
>> - {
>> - tree type = TREE_TYPE (op0);
>> - tree tmin = wide_int_to_tree (type, vr->lower_bound ());
>> - tree tmax = wide_int_to_tree (type, vr->upper_bound ());
>> - if (compare_values (tmin, min) == 1)
>> - min = tmin;
>> - if (compare_values (tmax, max) == -1)
>> - max = tmax;
>> -
>> - /* If the new min/max values have converged to a single value,
>> - then there is only one value which can satisfy the condition,
>> - return that value. */
>> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
>> - return min;
>> + vr.intersect (vr1);
>> + tree newop1;
>> + /* If the updated range is just a singleton, then we can just do a comparison */
>> + if (vr.singleton_p (&newop1))
>> + return newop1;
>> }
>> return NULL;
>> }
>> @@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
>> && cond_code != EQ_EXPR
>> && TREE_CODE (op0) == SSA_NAME
>> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
>> - && is_gimple_min_invariant (op1))
>> + && TREE_CODE (op1) == INTEGER_CST)
>> {
>> - value_range vr;
>> + Value_Range vr (TREE_TYPE (op0));
OK, so we know they are integers.. you could just iuse int_range_max
vr; if you want to stick to integers
>>
>> if (!query->range_of_expr (vr, op0, stmt))
>> vr.set_undefined ();
>> @@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
>> able to simplify this conditional. */
>> if (!vr.undefined_p () && !vr.varying_p ())
>> {
>> - tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
>> + tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
>> + true);
>> if (new_tree)
>> {
>> cond_code = EQ_EXPR;
>> op1 = new_tree;
>> happened = true;
>> }
>> -
>> - /* Try again after inverting the condition. We only deal
>> - with integral types here, so no need to worry about
>> - issues with inverting FP comparisons. */
>> - new_tree = test_for_singularity
>> - (invert_tree_comparison (cond_code, false),
>> - op0, op1, &vr);
>> + /* Try again after inverting the condition. */
>> + new_tree = test_for_singularity (cond_code, op0, op1, vr,
>> + false);
>> if (new_tree)
>> {
>> cond_code = NE_EXPR;
>> --
>> 2.31.1
>>
Other than that, LGTM.
Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 15:07 ` Andrew MacLeod
@ 2023-08-21 21:00 ` Andrew Pinski
2023-09-01 6:40 ` Andrew Pinski
1 sibling, 0 replies; 13+ messages in thread
From: Andrew Pinski @ 2023-08-21 21:00 UTC (permalink / raw)
To: Andrew MacLeod; +Cc: Richard Biener, Andrew Pinski, Aldy Hernandez, gcc-patches
On Fri, Aug 11, 2023 at 8:08 AM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> On 8/11/23 05:51, Richard Biener wrote:
> > On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >> So it turns out there was a simplier way of starting to
> >> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> >> That was rewrite test_for_singularity to use range_op_handler
> >> and Value_Range.
> >>
> >> This patch implements that and
> >>
> >> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > I'm hoping Andrew/Aldy can have a look here.
> >
> > Richard.
> >
> >> gcc/ChangeLog:
> >>
> >> * vr-values.cc (test_for_singularity): Add edge argument
> >> and rewrite using range_op_handler.
> >> (simplify_compare_using_range_pairs): Use Value_Range
> >> instead of value_range and update test_for_singularity call.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * gcc.dg/tree-ssa/vrp124.c: New test.
> >> * gcc.dg/tree-ssa/vrp125.c: New test.
> >> ---
> >> gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++
> >> gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++
> >> gcc/vr-values.cc | 91 ++++++++------------------
> >> 3 files changed, 114 insertions(+), 65 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> new file mode 100644
> >> index 00000000000..6ccbda35d1b
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> @@ -0,0 +1,44 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> >> +
> >> +/* Should be optimized to a == -100 */
> >> +int g(int a)
> >> +{
> >> + if (a == -100 || a >= 0)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 0;
> >> +}
> >> +
> >> +/* Should optimize to a == 0 */
> >> +int f(int a)
> >> +{
> >> + if (a == 0 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 50;
> >> +}
> >> +
> >> +/* Should be optimized to a == 0. */
> >> +int f2(int a)
> >> +{
> >> + if (a == 0 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 100;
> >> +}
> >> +
> >> +/* Should optimize to a == 100 */
> >> +int f1(int a)
> >> +{
> >> + if (a < 0 || a == 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a > 50;
> >> +}
> >> +
> >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >> new file mode 100644
> >> index 00000000000..f6c2f8e35f1
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >> @@ -0,0 +1,44 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> >> +
> >> +/* Should be optimized to a == -100 */
> >> +int g(int a)
> >> +{
> >> + if (a == -100 || a == -50 || a >= 0)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < -50;
> >> +}
> >> +
> >> +/* Should optimize to a == 0 */
> >> +int f(int a)
> >> +{
> >> + if (a == 0 || a == 50 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 50;
> >> +}
> >> +
> >> +/* Should be optimized to a == 0. */
> >> +int f2(int a)
> >> +{
> >> + if (a == 0 || a == 50 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 25;
> >> +}
> >> +
> >> +/* Should optimize to a == 100 */
> >> +int f1(int a)
> >> +{
> >> + if (a < 0 || a == 50 || a == 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a > 50;
> >> +}
> >> +
> >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> >> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> >> index a4fddd62841..7004b0224bd 100644
> >> --- a/gcc/vr-values.cc
> >> +++ b/gcc/vr-values.cc
> >> @@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> >> a known value range VR.
> >>
> >> If there is one and only one value which will satisfy the
> >> - conditional, then return that value. Else return NULL.
> >> -
> >> - If signed overflow must be undefined for the value to satisfy
> >> - the conditional, then set *STRICT_OVERFLOW_P to true. */
> >> + conditional on the EDGE, then return that value.
> >> + Else return NULL. */
> >>
> >> static tree
> >> test_for_singularity (enum tree_code cond_code, tree op0,
> >> - tree op1, const value_range *vr)
> >> + tree op1, Value_Range vr, bool edge)
>
> VR should be a "vrange &". THis is the top level base class for all
> ranges of all types/kinds, and what we usually pass values around as if
> we want tohem to be any kind. If this is inetger only, we'd pass a an
> 'irange &'
>
> Value_Range is the opposite. Its the sink that contains one of each kind
> of range and can switch around between them as needed. You do not want
> to pass that by value! The generic engine uses these so it can suppose
> floats. int, pointers, whatever...
>
> >> {
> >> - tree min = NULL;
> >> - tree max = NULL;
> >> -
> >> - /* Extract minimum/maximum values which satisfy the conditional as it was
> >> - written. */
> >> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> >> + /* This is already a singularity. */
> >> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> >> + return NULL;
> >> + auto range_op = range_op_handler (cond_code);
> >> + int_range<2> op1_range (TREE_TYPE (op0));
> >> + wide_int w = wi::to_wide (op1);
> >> + op1_range.set (TREE_TYPE (op1), w, w);
>
> If this is only going to work with integers, you might want to check
> that somewhere or switch to irange and int_range_max..
>
> You can make it work with any kind (if you know op1 is a constant) by
> simply doing
>
> Value_Range op1_range (TREE_TYPE (op1))
> get_global_range_query->range_of_expr (op1_range, op1)
>
> That will convert trees to a the appropriate range... THis is also true
> for integer constants... but you can also just do the WI conversion like
> you do.
>
> The routine also get confusing to read because it passes in op0 and
> op1, but of course ranger uses op1 and op2 nomenclature, and it looks a
> bit confusing :-P I'd change the operands passed in to op1 and op2 if
> we are rewriting the routine.
>
> >> + Value_Range vr1(TREE_TYPE (op0));
> >> + if (range_op.op1_range (vr1, TREE_TYPE (op0),
> >> + edge ? range_true () : range_false (),
> >> + op1_range))
>
> IF you decide to stick with integers, then you can just make vr1 an
> int_range_max vr1;
> You don't need the type in the declaration if you know its an irange.
> Value_Range needs a type because it needs to know if its an integr or a
> floating point (or whatever) that it is going ot be used as.
> >> {
> >> - min = TYPE_MIN_VALUE (TREE_TYPE (op0));
> >> -
> >> - max = op1;
> >> - if (cond_code == LT_EXPR)
> >> - {
> >> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> >> - max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
> >> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> >> - if (EXPR_P (max))
> >> - suppress_warning (max, OPT_Woverflow);
> >> - }
> >> - }
> >> - else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
> >> - {
> >> - max = TYPE_MAX_VALUE (TREE_TYPE (op0));
> >> -
> >> - min = op1;
> >> - if (cond_code == GT_EXPR)
> >> - {
> >> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> >> - min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
> >> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> >> - if (EXPR_P (min))
> >> - suppress_warning (min, OPT_Woverflow);
> >> - }
> >> - }
> >> -
> >> - /* Now refine the minimum and maximum values using any
> >> - value range information we have for op0. */
> >> - if (min && max)
> >> - {
> >> - tree type = TREE_TYPE (op0);
> >> - tree tmin = wide_int_to_tree (type, vr->lower_bound ());
> >> - tree tmax = wide_int_to_tree (type, vr->upper_bound ());
> >> - if (compare_values (tmin, min) == 1)
> >> - min = tmin;
> >> - if (compare_values (tmax, max) == -1)
> >> - max = tmax;
> >> -
> >> - /* If the new min/max values have converged to a single value,
> >> - then there is only one value which can satisfy the condition,
> >> - return that value. */
> >> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> >> - return min;
> >> + vr.intersect (vr1);
> >> + tree newop1;
> >> + /* If the updated range is just a singleton, then we can just do a comparison */
> >> + if (vr.singleton_p (&newop1))
> >> + return newop1;
> >> }
> >> return NULL;
> >> }
> >> @@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> >> && cond_code != EQ_EXPR
> >> && TREE_CODE (op0) == SSA_NAME
> >> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> >> - && is_gimple_min_invariant (op1))
> >> + && TREE_CODE (op1) == INTEGER_CST)
> >> {
> >> - value_range vr;
> >> + Value_Range vr (TREE_TYPE (op0));
> OK, so we know they are integers.. you could just iuse int_range_max
> vr; if you want to stick to integers
> >>
> >> if (!query->range_of_expr (vr, op0, stmt))
> >> vr.set_undefined ();
> >> @@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> >> able to simplify this conditional. */
> >> if (!vr.undefined_p () && !vr.varying_p ())
> >> {
> >> - tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
> >> + tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
> >> + true);
> >> if (new_tree)
> >> {
> >> cond_code = EQ_EXPR;
> >> op1 = new_tree;
> >> happened = true;
> >> }
> >> -
> >> - /* Try again after inverting the condition. We only deal
> >> - with integral types here, so no need to worry about
> >> - issues with inverting FP comparisons. */
> >> - new_tree = test_for_singularity
> >> - (invert_tree_comparison (cond_code, false),
> >> - op0, op1, &vr);
> >> + /* Try again after inverting the condition. */
> >> + new_tree = test_for_singularity (cond_code, op0, op1, vr,
> >> + false);
> >> if (new_tree)
> >> {
> >> cond_code = NE_EXPR;
> >> --
> >> 2.31.1
> >>
> Other than that, LGTM.
I am going to update this patch based on the review here; I just need
a few more days to do it.
Thanks,
Andrew
>
> Andrew
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-08-11 15:07 ` Andrew MacLeod
2023-08-21 21:00 ` Andrew Pinski
@ 2023-09-01 6:40 ` Andrew Pinski
2023-09-07 14:02 ` Andrew MacLeod
1 sibling, 1 reply; 13+ messages in thread
From: Andrew Pinski @ 2023-09-01 6:40 UTC (permalink / raw)
To: Andrew MacLeod; +Cc: Richard Biener, Andrew Pinski, Aldy Hernandez, gcc-patches
On Fri, Aug 11, 2023 at 8:08 AM Andrew MacLeod via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> On 8/11/23 05:51, Richard Biener wrote:
> > On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >> So it turns out there was a simplier way of starting to
> >> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> >> That was rewrite test_for_singularity to use range_op_handler
> >> and Value_Range.
> >>
> >> This patch implements that and
> >>
> >> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> > I'm hoping Andrew/Aldy can have a look here.
> >
> > Richard.
> >
> >> gcc/ChangeLog:
> >>
> >> * vr-values.cc (test_for_singularity): Add edge argument
> >> and rewrite using range_op_handler.
> >> (simplify_compare_using_range_pairs): Use Value_Range
> >> instead of value_range and update test_for_singularity call.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >> * gcc.dg/tree-ssa/vrp124.c: New test.
> >> * gcc.dg/tree-ssa/vrp125.c: New test.
> >> ---
> >> gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 +++++++++++++
> >> gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 +++++++++++++
> >> gcc/vr-values.cc | 91 ++++++++------------------
> >> 3 files changed, 114 insertions(+), 65 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >>
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> new file mode 100644
> >> index 00000000000..6ccbda35d1b
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
> >> @@ -0,0 +1,44 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> >> +
> >> +/* Should be optimized to a == -100 */
> >> +int g(int a)
> >> +{
> >> + if (a == -100 || a >= 0)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 0;
> >> +}
> >> +
> >> +/* Should optimize to a == 0 */
> >> +int f(int a)
> >> +{
> >> + if (a == 0 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 50;
> >> +}
> >> +
> >> +/* Should be optimized to a == 0. */
> >> +int f2(int a)
> >> +{
> >> + if (a == 0 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 100;
> >> +}
> >> +
> >> +/* Should optimize to a == 100 */
> >> +int f1(int a)
> >> +{
> >> + if (a < 0 || a == 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a > 50;
> >> +}
> >> +
> >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> >> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >> new file mode 100644
> >> index 00000000000..f6c2f8e35f1
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
> >> @@ -0,0 +1,44 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fdump-tree-optimized" } */
> >> +
> >> +/* Should be optimized to a == -100 */
> >> +int g(int a)
> >> +{
> >> + if (a == -100 || a == -50 || a >= 0)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < -50;
> >> +}
> >> +
> >> +/* Should optimize to a == 0 */
> >> +int f(int a)
> >> +{
> >> + if (a == 0 || a == 50 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 50;
> >> +}
> >> +
> >> +/* Should be optimized to a == 0. */
> >> +int f2(int a)
> >> +{
> >> + if (a == 0 || a == 50 || a > 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a < 25;
> >> +}
> >> +
> >> +/* Should optimize to a == 100 */
> >> +int f1(int a)
> >> +{
> >> + if (a < 0 || a == 50 || a == 100)
> >> + ;
> >> + else
> >> + return 0;
> >> + return a > 50;
> >> +}
> >> +
> >> +/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
> >> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> >> index a4fddd62841..7004b0224bd 100644
> >> --- a/gcc/vr-values.cc
> >> +++ b/gcc/vr-values.cc
> >> @@ -907,66 +907,30 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> >> a known value range VR.
> >>
> >> If there is one and only one value which will satisfy the
> >> - conditional, then return that value. Else return NULL.
> >> -
> >> - If signed overflow must be undefined for the value to satisfy
> >> - the conditional, then set *STRICT_OVERFLOW_P to true. */
> >> + conditional on the EDGE, then return that value.
> >> + Else return NULL. */
> >>
> >> static tree
> >> test_for_singularity (enum tree_code cond_code, tree op0,
> >> - tree op1, const value_range *vr)
> >> + tree op1, Value_Range vr, bool edge)
>
> VR should be a "vrange &". THis is the top level base class for all
> ranges of all types/kinds, and what we usually pass values around as if
> we want tohem to be any kind. If this is inetger only, we'd pass a an
> 'irange &'
>
> Value_Range is the opposite. Its the sink that contains one of each kind
> of range and can switch around between them as needed. You do not want
> to pass that by value! The generic engine uses these so it can suppose
> floats. int, pointers, whatever...
>
> >> {
> >> - tree min = NULL;
> >> - tree max = NULL;
> >> -
> >> - /* Extract minimum/maximum values which satisfy the conditional as it was
> >> - written. */
> >> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> >> + /* This is already a singularity. */
> >> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> >> + return NULL;
> >> + auto range_op = range_op_handler (cond_code);
> >> + int_range<2> op1_range (TREE_TYPE (op0));
> >> + wide_int w = wi::to_wide (op1);
> >> + op1_range.set (TREE_TYPE (op1), w, w);
>
> If this is only going to work with integers, you might want to check
> that somewhere or switch to irange and int_range_max..
>
> You can make it work with any kind (if you know op1 is a constant) by
> simply doing
>
> Value_Range op1_range (TREE_TYPE (op1))
> get_global_range_query->range_of_expr (op1_range, op1)
>
> That will convert trees to a the appropriate range... THis is also true
> for integer constants... but you can also just do the WI conversion like
> you do.
>
> The routine also get confusing to read because it passes in op0 and
> op1, but of course ranger uses op1 and op2 nomenclature, and it looks a
> bit confusing :-P I'd change the operands passed in to op1 and op2 if
> we are rewriting the routine.
Ranger using the nomenclature of op1/op2 and gimple is inconsistent
with trees and other parts of GCC.
It seems like we have to live with this inconsistency now too.
Renaming things in this one function to op1/op2 might be ok but the
rest of the file uses op0/op1 too; most likely because it was
originally written before gimple.
I think it would be good to have this written in the coding style,
which way should we have it for new code; if we start at 0 or 1 for
operands. It might reduce differences based on who wrote which part
(and even to some extent when). I don't really care which one is
picked as long as we pick one.
Thanks,
Andrew Pinski
>
> >> + Value_Range vr1(TREE_TYPE (op0));
> >> + if (range_op.op1_range (vr1, TREE_TYPE (op0),
> >> + edge ? range_true () : range_false (),
> >> + op1_range))
>
> IF you decide to stick with integers, then you can just make vr1 an
> int_range_max vr1;
> You don't need the type in the declaration if you know its an irange.
> Value_Range needs a type because it needs to know if its an integr or a
> floating point (or whatever) that it is going ot be used as.
> >> {
> >> - min = TYPE_MIN_VALUE (TREE_TYPE (op0));
> >> -
> >> - max = op1;
> >> - if (cond_code == LT_EXPR)
> >> - {
> >> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> >> - max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
> >> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> >> - if (EXPR_P (max))
> >> - suppress_warning (max, OPT_Woverflow);
> >> - }
> >> - }
> >> - else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
> >> - {
> >> - max = TYPE_MAX_VALUE (TREE_TYPE (op0));
> >> -
> >> - min = op1;
> >> - if (cond_code == GT_EXPR)
> >> - {
> >> - tree one = build_int_cst (TREE_TYPE (op0), 1);
> >> - min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
> >> - /* Signal to compare_values_warnv this expr doesn't overflow. */
> >> - if (EXPR_P (min))
> >> - suppress_warning (min, OPT_Woverflow);
> >> - }
> >> - }
> >> -
> >> - /* Now refine the minimum and maximum values using any
> >> - value range information we have for op0. */
> >> - if (min && max)
> >> - {
> >> - tree type = TREE_TYPE (op0);
> >> - tree tmin = wide_int_to_tree (type, vr->lower_bound ());
> >> - tree tmax = wide_int_to_tree (type, vr->upper_bound ());
> >> - if (compare_values (tmin, min) == 1)
> >> - min = tmin;
> >> - if (compare_values (tmax, max) == -1)
> >> - max = tmax;
> >> -
> >> - /* If the new min/max values have converged to a single value,
> >> - then there is only one value which can satisfy the condition,
> >> - return that value. */
> >> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> >> - return min;
> >> + vr.intersect (vr1);
> >> + tree newop1;
> >> + /* If the updated range is just a singleton, then we can just do a comparison */
> >> + if (vr.singleton_p (&newop1))
> >> + return newop1;
> >> }
> >> return NULL;
> >> }
> >> @@ -1224,9 +1188,9 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> >> && cond_code != EQ_EXPR
> >> && TREE_CODE (op0) == SSA_NAME
> >> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> >> - && is_gimple_min_invariant (op1))
> >> + && TREE_CODE (op1) == INTEGER_CST)
> >> {
> >> - value_range vr;
> >> + Value_Range vr (TREE_TYPE (op0));
> OK, so we know they are integers.. you could just iuse int_range_max
> vr; if you want to stick to integers
> >>
> >> if (!query->range_of_expr (vr, op0, stmt))
> >> vr.set_undefined ();
> >> @@ -1235,20 +1199,17 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
> >> able to simplify this conditional. */
> >> if (!vr.undefined_p () && !vr.varying_p ())
> >> {
> >> - tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
> >> + tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
> >> + true);
> >> if (new_tree)
> >> {
> >> cond_code = EQ_EXPR;
> >> op1 = new_tree;
> >> happened = true;
> >> }
> >> -
> >> - /* Try again after inverting the condition. We only deal
> >> - with integral types here, so no need to worry about
> >> - issues with inverting FP comparisons. */
> >> - new_tree = test_for_singularity
> >> - (invert_tree_comparison (cond_code, false),
> >> - op0, op1, &vr);
> >> + /* Try again after inverting the condition. */
> >> + new_tree = test_for_singularity (cond_code, op0, op1, vr,
> >> + false);
> >> if (new_tree)
> >> {
> >> cond_code = NE_EXPR;
> >> --
> >> 2.31.1
> >>
> Other than that, LGTM.
>
> Andrew
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-09-01 6:40 ` Andrew Pinski
@ 2023-09-07 14:02 ` Andrew MacLeod
0 siblings, 0 replies; 13+ messages in thread
From: Andrew MacLeod @ 2023-09-07 14:02 UTC (permalink / raw)
To: Andrew Pinski; +Cc: Richard Biener, Andrew Pinski, Aldy Hernandez, gcc-patches
On 9/1/23 02:40, Andrew Pinski wrote:
> On Fri, Aug 11, 2023 at 8:08 AM Andrew MacLeod via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> If this is only going to work with integers, you might want to check
>> that somewhere or switch to irange and int_range_max..
>>
>> You can make it work with any kind (if you know op1 is a constant) by
>> simply doing
>>
>> Value_Range op1_range (TREE_TYPE (op1))
>> get_global_range_query->range_of_expr (op1_range, op1)
>>
>> That will convert trees to a the appropriate range... THis is also true
>> for integer constants... but you can also just do the WI conversion like
>> you do.
>>
>> The routine also get confusing to read because it passes in op0 and
>> op1, but of course ranger uses op1 and op2 nomenclature, and it looks a
>> bit confusing :-P I'd change the operands passed in to op1 and op2 if
>> we are rewriting the routine.
> Ranger using the nomenclature of op1/op2 and gimple is inconsistent
> with trees and other parts of GCC.
> It seems like we have to live with this inconsistency now too.
> Renaming things in this one function to op1/op2 might be ok but the
> rest of the file uses op0/op1 too; most likely because it was
> originally written before gimple.
>
> I think it would be good to have this written in the coding style,
> which way should we have it for new code; if we start at 0 or 1 for
> operands. It might reduce differences based on who wrote which part
> (and even to some extent when). I don't really care which one is
> picked as long as we pick one.
>
> Thanks,
> Andrew Pinski
>
I certainly wont argue it would be good to be consistent, but of course
its quite prevalent. Perhaps we should rewrite vr-values.cc to change
the terminology in one patch?
long term some of it is likely to get absorbed into rangeops, and what
isn't could/should be made vrange/irange aware... no one has gotten to
it yet. we could change the terminology as the routines are reworked too...
Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement
2023-08-11 9:15 [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Andrew Pinski
2023-08-11 9:15 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
@ 2023-08-11 9:50 ` Richard Biener
1 sibling, 0 replies; 13+ messages in thread
From: Richard Biener @ 2023-08-11 9:50 UTC (permalink / raw)
To: Andrew Pinski; +Cc: gcc-patches
On Fri, Aug 11, 2023 at 11:17 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> So with my next VRP patch, VRP causes:
> ```
> # c$_M_value_18 = PHI <-1(3), 0(2), 1(4)>
> _11 = (unsigned int) c$_M_value_18;
> _16 = _11 <= 1;
> ```
> To be changed to:
> ```
> # c$_M_value_18 = PHI <-1(3), 0(2), 1(4)>
> _11 = (unsigned int) c$_M_value_18;
> _16 = _11 != 4294967295;
> ```
>
> So let's add support for the above.
> A few changes was needed, first to change
> the range check of the rhs of the comparison to possibly
> integer_all_onesp also.
>
> The next is to add support for the cast and EQ/NE case.
>
> Note on the testcases pr110984-1.c is basically pr94589-2.c but
> with what the C++ code is doing with the signed char type;
> pr110984-2.c is pr110984-1.c with the cast added to give an
> explicit testcase to test against.
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
LGTM.
> Thanks,
> Andrew Pinski
>
> PR tree-optimization/110984
>
> gcc/ChangeLog:
>
> * tree-ssa-phiopt.cc (spaceship_replacement): Add support for
> NE/EQ for the cast case.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/pr110984-1.c: New test.
> * gcc.dg/pr110984-2.c: New test.
> ---
> gcc/testsuite/gcc.dg/pr110984-1.c | 37 +++++++++++++++++++++++++++++++
> gcc/testsuite/gcc.dg/pr110984-2.c | 21 ++++++++++++++++++
> gcc/tree-ssa-phiopt.cc | 19 +++++++++++++---
> 3 files changed, 74 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr110984-1.c
> create mode 100644 gcc/testsuite/gcc.dg/pr110984-2.c
>
> diff --git a/gcc/testsuite/gcc.dg/pr110984-1.c b/gcc/testsuite/gcc.dg/pr110984-1.c
> new file mode 100644
> index 00000000000..85b19eb8279
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr110984-1.c
> @@ -0,0 +1,37 @@
> +/* PR tree-optimization/110984 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -g0 -ffast-math -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-times "\[ij]_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) \[ij]_\[0-9]+\\(D\\)" 14 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times "i_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) 5\\.0" 14 "optimized" } } */
> +
> +/* This is similar to pr94589-2.c except use signed char as the type for the [-1,2] case */
> +
> +#define A __attribute__((noipa))
> +A int f1 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == 0; }
> +A int f2 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != 0; }
> +A int f3 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c > 0; }
> +A int f4 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c < 0; }
> +A int f5 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c >= 0; }
> +A int f6 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c <= 0; }
> +A int f7 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == -1; }
> +A int f8 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != -1; }
> +A int f9 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c > -1; }
> +A int f10 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c <= -1; }
> +A int f11 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c == 1; }
> +A int f12 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c != 1; }
> +A int f13 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c < 1; }
> +A int f14 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; return c >= 1; }
> +A int f15 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == 0; }
> +A int f16 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != 0; }
> +A int f17 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c > 0; }
> +A int f18 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c < 0; }
> +A int f19 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c >= 0; }
> +A int f20 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c <= 0; }
> +A int f21 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == -1; }
> +A int f22 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != -1; }
> +A int f23 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c > -1; }
> +A int f24 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c <= -1; }
> +A int f25 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c == 1; }
> +A int f26 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c != 1; }
> +A int f27 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c < 1; }
> +A int f28 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; return c >= 1; }
> diff --git a/gcc/testsuite/gcc.dg/pr110984-2.c b/gcc/testsuite/gcc.dg/pr110984-2.c
> new file mode 100644
> index 00000000000..cddce045745
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr110984-2.c
> @@ -0,0 +1,21 @@
> +/* PR tree-optimization/110984 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -g0 -ffast-math -fdump-tree-optimized" } */
> +/* { dg-final { scan-tree-dump-times "\[ij]_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) \[ij]_\[0-9]+\\(D\\)" 6 "optimized" } } */
> +/* { dg-final { scan-tree-dump-times "i_\[0-9]+\\(D\\) (?:<|<=|==|!=|>|>=) 5\\.0" 6 "optimized" } } */
> +
> +/* This is similar to pr94589-2.c except use signed char and use unsigned types to check against the variable, which means removing the non !=/== comparisons. */
> +
> +#define A __attribute__((noipa))
> +A int f1 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == 0; }
> +A int f2 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != 0; }
> +A int f7 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == -1; }
> +A int f8 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != -1; }
> +A int f11 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t == 1; }
> +A int f12 (double i, double j) { signed char c; if (i == j) c = 0; else if (i < j) c = -1; else if (i > j) c = 1; else c = 2; unsigned t = c; return t != 1; }
> +A int f15 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == 0; }
> +A int f16 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != 0; }
> +A int f21 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == -1; }
> +A int f22 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != -1; }
> +A int f25 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t == 1; }
> +A int f26 (double i) { signed char c; if (i == 5.0) c = 0; else if (i < 5.0) c = -1; else if (i > 5.0) c = 1; else c = 2; unsigned t = c; return t != 1; }
> diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> index 485662dfcc7..5f24d62c49a 100644
> --- a/gcc/tree-ssa-phiopt.cc
> +++ b/gcc/tree-ssa-phiopt.cc
> @@ -2346,7 +2346,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
> }
> if (lhs != (orig_use_lhs ? orig_use_lhs : phires)
> || !tree_fits_shwi_p (rhs)
> - || !IN_RANGE (tree_to_shwi (rhs), -1, 1))
> + || !(IN_RANGE (tree_to_shwi (rhs), 0, 1)
> + || integer_all_onesp (rhs)))
> return false;
>
> if (is_cast)
> @@ -2356,9 +2357,20 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
> /* As for -ffast-math we assume the 2 return to be
> impossible, canonicalize (unsigned) res <= 1U or
> (unsigned) res < 2U into res >= 0 and (unsigned) res > 1U
> - or (unsigned) res >= 2U as res < 0. */
> + or (unsigned) res >= 2U as res < 0.
> + Sometimes we get (unsigned)res != N. Support those cases too. */
> switch (cmp)
> {
> + case NE_EXPR:
> + case EQ_EXPR:
> + {
> + tree newrhs = fold_convert (TREE_TYPE (phires), rhs);
> + tree tmp = fold_convert (TREE_TYPE (rhs), newrhs);
> + if (!tree_int_cst_equal (rhs, tmp))
> + return false;
> + rhs = newrhs;
> + break;
> + }
> case LE_EXPR:
> if (!integer_onep (rhs))
> return false;
> @@ -2382,7 +2394,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb,
> default:
> return false;
> }
> - rhs = build_zero_cst (TREE_TYPE (phires));
> + if (cmp != EQ_EXPR && cmp != NE_EXPR)
> + rhs = build_zero_cst (TREE_TYPE (phires));
> }
> else if (orig_use_lhs)
> {
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] VR-VALUES: Rename op0/op1 to op1/op2 for test_for_singularity
@ 2023-09-01 17:30 Andrew Pinski
2023-09-01 17:30 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Pinski @ 2023-09-01 17:30 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
As requested and make easier to understand with the new ranger
code, rename the arguments op0/op1 to op1/op2.
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions
gcc/ChangeLog:
* vr-values.cc (test_for_singularity): Rename
arguments op0/op1 to op1/op2.
---
gcc/vr-values.cc | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
index a4fddd62841..52ab4fe6109 100644
--- a/gcc/vr-values.cc
+++ b/gcc/vr-values.cc
@@ -903,7 +903,7 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
return true;
}
-/* We are comparing trees OP0 and OP1 using COND_CODE. OP0 has
+/* We are comparing trees OP1 and OP2 using COND_CODE. OP1 has
a known value range VR.
If there is one and only one value which will satisfy the
@@ -913,8 +913,8 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
the conditional, then set *STRICT_OVERFLOW_P to true. */
static tree
-test_for_singularity (enum tree_code cond_code, tree op0,
- tree op1, const value_range *vr)
+test_for_singularity (enum tree_code cond_code, tree op1,
+ tree op2, const value_range *vr)
{
tree min = NULL;
tree max = NULL;
@@ -923,13 +923,13 @@ test_for_singularity (enum tree_code cond_code, tree op0,
written. */
if (cond_code == LE_EXPR || cond_code == LT_EXPR)
{
- min = TYPE_MIN_VALUE (TREE_TYPE (op0));
+ min = TYPE_MIN_VALUE (TREE_TYPE (op1));
- max = op1;
+ max = op2;
if (cond_code == LT_EXPR)
{
- tree one = build_int_cst (TREE_TYPE (op0), 1);
- max = fold_build2 (MINUS_EXPR, TREE_TYPE (op0), max, one);
+ tree one = build_int_cst (TREE_TYPE (op1), 1);
+ max = fold_build2 (MINUS_EXPR, TREE_TYPE (op1), max, one);
/* Signal to compare_values_warnv this expr doesn't overflow. */
if (EXPR_P (max))
suppress_warning (max, OPT_Woverflow);
@@ -937,13 +937,13 @@ test_for_singularity (enum tree_code cond_code, tree op0,
}
else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
{
- max = TYPE_MAX_VALUE (TREE_TYPE (op0));
+ max = TYPE_MAX_VALUE (TREE_TYPE (op1));
- min = op1;
+ min = op2;
if (cond_code == GT_EXPR)
{
- tree one = build_int_cst (TREE_TYPE (op0), 1);
- min = fold_build2 (PLUS_EXPR, TREE_TYPE (op0), min, one);
+ tree one = build_int_cst (TREE_TYPE (op1), 1);
+ min = fold_build2 (PLUS_EXPR, TREE_TYPE (op1), min, one);
/* Signal to compare_values_warnv this expr doesn't overflow. */
if (EXPR_P (min))
suppress_warning (min, OPT_Woverflow);
@@ -951,10 +951,10 @@ test_for_singularity (enum tree_code cond_code, tree op0,
}
/* Now refine the minimum and maximum values using any
- value range information we have for op0. */
+ value range information we have for op1. */
if (min && max)
{
- tree type = TREE_TYPE (op0);
+ tree type = TREE_TYPE (op1);
tree tmin = wide_int_to_tree (type, vr->lower_bound ());
tree tmax = wide_int_to_tree (type, vr->upper_bound ());
if (compare_values (tmin, min) == 1)
--
2.31.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-09-01 17:30 [PATCH 1/2] VR-VALUES: Rename op0/op1 to op1/op2 for test_for_singularity Andrew Pinski
@ 2023-09-01 17:30 ` Andrew Pinski
2023-09-05 6:06 ` Jeff Law
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Pinski @ 2023-09-01 17:30 UTC (permalink / raw)
To: gcc-patches; +Cc: Andrew Pinski
So it turns out there was a simplier way of starting to
improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
That was rewrite test_for_singularity to use range_op_handler
and Value_Range.
This patch implements that and
OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
gcc/ChangeLog:
* vr-values.cc (test_for_singularity): Add edge argument
and rewrite using range_op_handler.
(simplify_compare_using_range_pairs): Use Value_Range
instead of value_range and update test_for_singularity call.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/vrp124.c: New test.
* gcc.dg/tree-ssa/vrp125.c: New test.
---
gcc/testsuite/gcc.dg/tree-ssa/vrp124.c | 44 ++++++++++++
gcc/testsuite/gcc.dg/tree-ssa/vrp125.c | 44 ++++++++++++
gcc/vr-values.cc | 99 ++++++++------------------
3 files changed, 117 insertions(+), 70 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
new file mode 100644
index 00000000000..6ccbda35d1b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp124.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+/* Should be optimized to a == -100 */
+int g(int a)
+{
+ if (a == -100 || a >= 0)
+ ;
+ else
+ return 0;
+ return a < 0;
+}
+
+/* Should optimize to a == 0 */
+int f(int a)
+{
+ if (a == 0 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 50;
+}
+
+/* Should be optimized to a == 0. */
+int f2(int a)
+{
+ if (a == 0 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 100;
+}
+
+/* Should optimize to a == 100 */
+int f1(int a)
+{
+ if (a < 0 || a == 100)
+ ;
+ else
+ return 0;
+ return a > 50;
+}
+
+/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
new file mode 100644
index 00000000000..f6c2f8e35f1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp125.c
@@ -0,0 +1,44 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+/* Should be optimized to a == -100 */
+int g(int a)
+{
+ if (a == -100 || a == -50 || a >= 0)
+ ;
+ else
+ return 0;
+ return a < -50;
+}
+
+/* Should optimize to a == 0 */
+int f(int a)
+{
+ if (a == 0 || a == 50 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 50;
+}
+
+/* Should be optimized to a == 0. */
+int f2(int a)
+{
+ if (a == 0 || a == 50 || a > 100)
+ ;
+ else
+ return 0;
+ return a < 25;
+}
+
+/* Should optimize to a == 100 */
+int f1(int a)
+{
+ if (a < 0 || a == 50 || a == 100)
+ ;
+ else
+ return 0;
+ return a > 50;
+}
+
+/* { dg-final { scan-tree-dump-not "goto " "optimized" } } */
diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
index 52ab4fe6109..2474e57ee90 100644
--- a/gcc/vr-values.cc
+++ b/gcc/vr-values.cc
@@ -904,69 +904,33 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
}
/* We are comparing trees OP1 and OP2 using COND_CODE. OP1 has
- a known value range VR.
+ a known value range OP1_RANGE.
If there is one and only one value which will satisfy the
- conditional, then return that value. Else return NULL.
-
- If signed overflow must be undefined for the value to satisfy
- the conditional, then set *STRICT_OVERFLOW_P to true. */
+ conditional on the EDGE, then return that value.
+ Else return NULL. */
static tree
test_for_singularity (enum tree_code cond_code, tree op1,
- tree op2, const value_range *vr)
+ tree op2, const int_range_max &op1_range, bool edge)
{
- tree min = NULL;
- tree max = NULL;
-
- /* Extract minimum/maximum values which satisfy the conditional as it was
- written. */
- if (cond_code == LE_EXPR || cond_code == LT_EXPR)
+ /* This is already a singularity. */
+ if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
+ return NULL;
+ auto range_op = range_op_handler (cond_code);
+ wide_int w = wi::to_wide (op2);
+ int_range<1> op2_range (TREE_TYPE (op2), w, w);
+ int_range_max vr;
+ if (range_op.op1_range (vr, TREE_TYPE (op1),
+ edge ? range_true () : range_false (),
+ op2_range))
{
- min = TYPE_MIN_VALUE (TREE_TYPE (op1));
-
- max = op2;
- if (cond_code == LT_EXPR)
- {
- tree one = build_int_cst (TREE_TYPE (op1), 1);
- max = fold_build2 (MINUS_EXPR, TREE_TYPE (op1), max, one);
- /* Signal to compare_values_warnv this expr doesn't overflow. */
- if (EXPR_P (max))
- suppress_warning (max, OPT_Woverflow);
- }
- }
- else if (cond_code == GE_EXPR || cond_code == GT_EXPR)
- {
- max = TYPE_MAX_VALUE (TREE_TYPE (op1));
-
- min = op2;
- if (cond_code == GT_EXPR)
- {
- tree one = build_int_cst (TREE_TYPE (op1), 1);
- min = fold_build2 (PLUS_EXPR, TREE_TYPE (op1), min, one);
- /* Signal to compare_values_warnv this expr doesn't overflow. */
- if (EXPR_P (min))
- suppress_warning (min, OPT_Woverflow);
- }
- }
-
- /* Now refine the minimum and maximum values using any
- value range information we have for op1. */
- if (min && max)
- {
- tree type = TREE_TYPE (op1);
- tree tmin = wide_int_to_tree (type, vr->lower_bound ());
- tree tmax = wide_int_to_tree (type, vr->upper_bound ());
- if (compare_values (tmin, min) == 1)
- min = tmin;
- if (compare_values (tmax, max) == -1)
- max = tmax;
-
- /* If the new min/max values have converged to a single value,
- then there is only one value which can satisfy the condition,
- return that value. */
- if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
- return min;
+ int_range_max new_range = op1_range;
+ new_range.intersect (vr);
+ tree newop2;
+ /* If the updated range is just a singleton, then we can just do a comparison */
+ if (new_range.singleton_p (&newop2))
+ return newop2;
}
return NULL;
}
@@ -1224,31 +1188,26 @@ simplify_using_ranges::simplify_compare_using_ranges_1 (tree_code &cond_code, tr
&& cond_code != EQ_EXPR
&& TREE_CODE (op0) == SSA_NAME
&& INTEGRAL_TYPE_P (TREE_TYPE (op0))
- && is_gimple_min_invariant (op1))
+ && TREE_CODE (op1) == INTEGER_CST)
{
- value_range vr;
-
- if (!query->range_of_expr (vr, op0, stmt))
- vr.set_undefined ();
+ int_range_max vr;
/* If we have range information for OP0, then we might be
able to simplify this conditional. */
- if (!vr.undefined_p () && !vr.varying_p ())
+ if (query->range_of_expr (vr, op0, stmt)
+ && !vr.undefined_p () && !vr.varying_p ())
{
- tree new_tree = test_for_singularity (cond_code, op0, op1, &vr);
+ tree new_tree = test_for_singularity (cond_code, op0, op1, vr,
+ true);
if (new_tree)
{
cond_code = EQ_EXPR;
op1 = new_tree;
happened = true;
}
-
- /* Try again after inverting the condition. We only deal
- with integral types here, so no need to worry about
- issues with inverting FP comparisons. */
- new_tree = test_for_singularity
- (invert_tree_comparison (cond_code, false),
- op0, op1, &vr);
+ /* Try again after inverting the condition. */
+ new_tree = test_for_singularity (cond_code, op0, op1, vr,
+ false);
if (new_tree)
{
cond_code = NE_EXPR;
--
2.31.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-09-01 17:30 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
@ 2023-09-05 6:06 ` Jeff Law
2023-09-05 7:12 ` Andrew Pinski
0 siblings, 1 reply; 13+ messages in thread
From: Jeff Law @ 2023-09-05 6:06 UTC (permalink / raw)
To: Andrew Pinski, gcc-patches
On 9/1/23 11:30, Andrew Pinski via Gcc-patches wrote:
> So it turns out there was a simplier way of starting to
> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> That was rewrite test_for_singularity to use range_op_handler
> and Value_Range.
>
> This patch implements that and
>
> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>
> gcc/ChangeLog:
>
> * vr-values.cc (test_for_singularity): Add edge argument
> and rewrite using range_op_handler.
> (simplify_compare_using_range_pairs): Use Value_Range
> instead of value_range and update test_for_singularity call.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.dg/tree-ssa/vrp124.c: New test.
> * gcc.dg/tree-ssa/vrp125.c: New test.
> ---
> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> index 52ab4fe6109..2474e57ee90 100644
> --- a/gcc/vr-values.cc
> +++ b/gcc/vr-values.cc
> @@ -904,69 +904,33 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> }
>
> /* We are comparing trees OP1 and OP2 using COND_CODE. OP1 has
> - a known value range VR.
> + a known value range OP1_RANGE.
>
> If there is one and only one value which will satisfy the
> - conditional, then return that value. Else return NULL.
> -
> - If signed overflow must be undefined for the value to satisfy
> - the conditional, then set *STRICT_OVERFLOW_P to true. */
> + conditional on the EDGE, then return that value.
> + Else return NULL. */
>
> static tree
> test_for_singularity (enum tree_code cond_code, tree op1,
> - tree op2, const value_range *vr)
> + tree op2, const int_range_max &op1_range, bool edge)
> {
> - tree min = NULL;
> - tree max = NULL;
> -
> - /* Extract minimum/maximum values which satisfy the conditional as it was
> - written. */
> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> + /* This is already a singularity. */
> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> + return NULL;
I don't think this is necessarily the right thing to do for NE.
Consider if op1 has the range [0,1] and op2 has the value 1. If the
code is NE, then we should be able to return a singularity of 0 since
that's the only value for x where x ne 1 is true given the range for x.
I like what you're trying to do, it just needs a bit of refinement I think.
jeff
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-09-05 6:06 ` Jeff Law
@ 2023-09-05 7:12 ` Andrew Pinski
2023-09-29 20:17 ` Jeff Law
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Pinski @ 2023-09-05 7:12 UTC (permalink / raw)
To: Jeff Law; +Cc: Andrew Pinski, gcc-patches
On Mon, Sep 4, 2023 at 11:06 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 9/1/23 11:30, Andrew Pinski via Gcc-patches wrote:
> > So it turns out there was a simplier way of starting to
> > improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
> > That was rewrite test_for_singularity to use range_op_handler
> > and Value_Range.
> >
> > This patch implements that and
> >
> > OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >
> > gcc/ChangeLog:
> >
> > * vr-values.cc (test_for_singularity): Add edge argument
> > and rewrite using range_op_handler.
> > (simplify_compare_using_range_pairs): Use Value_Range
> > instead of value_range and update test_for_singularity call.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.dg/tree-ssa/vrp124.c: New test.
> > * gcc.dg/tree-ssa/vrp125.c: New test.
> > ---
>
> > diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
> > index 52ab4fe6109..2474e57ee90 100644
> > --- a/gcc/vr-values.cc
> > +++ b/gcc/vr-values.cc
> > @@ -904,69 +904,33 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
> > }
> >
> > /* We are comparing trees OP1 and OP2 using COND_CODE. OP1 has
> > - a known value range VR.
> > + a known value range OP1_RANGE.
> >
> > If there is one and only one value which will satisfy the
> > - conditional, then return that value. Else return NULL.
> > -
> > - If signed overflow must be undefined for the value to satisfy
> > - the conditional, then set *STRICT_OVERFLOW_P to true. */
> > + conditional on the EDGE, then return that value.
> > + Else return NULL. */
> >
> > static tree
> > test_for_singularity (enum tree_code cond_code, tree op1,
> > - tree op2, const value_range *vr)
> > + tree op2, const int_range_max &op1_range, bool edge)
> > {
> > - tree min = NULL;
> > - tree max = NULL;
> > -
> > - /* Extract minimum/maximum values which satisfy the conditional as it was
> > - written. */
> > - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
> > + /* This is already a singularity. */
> > + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
> > + return NULL;
> I don't think this is necessarily the right thing to do for NE.
>
> Consider if op1 has the range [0,1] and op2 has the value 1. If the
> code is NE, then we should be able to return a singularity of 0 since
> that's the only value for x where x ne 1 is true given the range for x.
The "false" edge singularity is already known when NE is supplied. I
don't think changing it to the "true" edge singularity will be helpful
all of the time; preferring the value of 0 is a different story.
But that is a different patch and for a different location rather than
inside VRP; it should be in either isel or expand (more likely isel).
Thanks,
Andrew
>
>
>
> I like what you're trying to do, it just needs a bit of refinement I think.
>
> jeff
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler
2023-09-05 7:12 ` Andrew Pinski
@ 2023-09-29 20:17 ` Jeff Law
0 siblings, 0 replies; 13+ messages in thread
From: Jeff Law @ 2023-09-29 20:17 UTC (permalink / raw)
To: Andrew Pinski; +Cc: Andrew Pinski, gcc-patches
On 9/5/23 01:12, Andrew Pinski wrote:
> On Mon, Sep 4, 2023 at 11:06 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 9/1/23 11:30, Andrew Pinski via Gcc-patches wrote:
>>> So it turns out there was a simplier way of starting to
>>> improve VRP to start to fix PR 110131, PR 108360, and PR 108397.
>>> That was rewrite test_for_singularity to use range_op_handler
>>> and Value_Range.
>>>
>>> This patch implements that and
>>>
>>> OK? Bootstrapped and tested on x86_64-linux-gnu with no regressions.
>>>
>>> gcc/ChangeLog:
>>>
>>> * vr-values.cc (test_for_singularity): Add edge argument
>>> and rewrite using range_op_handler.
>>> (simplify_compare_using_range_pairs): Use Value_Range
>>> instead of value_range and update test_for_singularity call.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * gcc.dg/tree-ssa/vrp124.c: New test.
>>> * gcc.dg/tree-ssa/vrp125.c: New test.
>>> ---
>>
>>> diff --git a/gcc/vr-values.cc b/gcc/vr-values.cc
>>> index 52ab4fe6109..2474e57ee90 100644
>>> --- a/gcc/vr-values.cc
>>> +++ b/gcc/vr-values.cc
>>> @@ -904,69 +904,33 @@ simplify_using_ranges::simplify_bit_ops_using_ranges
>>> }
>>>
>>> /* We are comparing trees OP1 and OP2 using COND_CODE. OP1 has
>>> - a known value range VR.
>>> + a known value range OP1_RANGE.
>>>
>>> If there is one and only one value which will satisfy the
>>> - conditional, then return that value. Else return NULL.
>>> -
>>> - If signed overflow must be undefined for the value to satisfy
>>> - the conditional, then set *STRICT_OVERFLOW_P to true. */
>>> + conditional on the EDGE, then return that value.
>>> + Else return NULL. */
>>>
>>> static tree
>>> test_for_singularity (enum tree_code cond_code, tree op1,
>>> - tree op2, const value_range *vr)
>>> + tree op2, const int_range_max &op1_range, bool edge)
>>> {
>>> - tree min = NULL;
>>> - tree max = NULL;
>>> -
>>> - /* Extract minimum/maximum values which satisfy the conditional as it was
>>> - written. */
>>> - if (cond_code == LE_EXPR || cond_code == LT_EXPR)
>>> + /* This is already a singularity. */
>>> + if (cond_code == NE_EXPR || cond_code == EQ_EXPR)
>>> + return NULL;
>> I don't think this is necessarily the right thing to do for NE.
>>
>> Consider if op1 has the range [0,1] and op2 has the value 1. If the
>> code is NE, then we should be able to return a singularity of 0 since
>> that's the only value for x where x ne 1 is true given the range for x.
>
> The "false" edge singularity is already known when NE is supplied. I
> don't think changing it to the "true" edge singularity will be helpful
> all of the time; preferring the value of 0 is a different story.
> But that is a different patch and for a different location rather than
> inside VRP; it should be in either isel or expand (more likely isel).
I forgot something critically important here. Specifically, this code
is supposed to be subsumed by Ranger.
The whole point of this routine is to rewrite to EQ/NE comparisons so
that we can expose equivalences on the true/false arm of the conditional
(and NE is just as important as EQ). It's not really about preferring
any particular value like 0.
The problem with this routine is it loses information after the code has
been transformed. Let's say we had a test x < 4. If we assume that VRP
is able to prove X doesn't have any of the values [MIN..3], then we can
change the test to x == 4 and propagate 4 for any uses of X in the true arm.
But on the false ARM we end up with x != 4 which is a wider range than
[5..MAX]. So if we were to instantiate a new Ranger after the
transformation we'd lose information on the false arm. More
importantly, I think the transformation was bad for either SCEV or loop
iteration analysis.
When Andrew, Aldy and I kicked this problem around the consensus was
that Ranger should find and propagate the equivalence, including making
it visible to jump threading. That should make the rewriting totally
unnecessary.
So the net is we really ought to be doing here is looking for cases
where this code actually helps code generation and if it does we need to
understand how/why as this code is supposed to go away.
Given you're already poking around in here, you might have such cases
handy :-) If you do, I'm sure Andrew, Aldy and I would love to see them.
jeff
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-09-29 20:17 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-11 9:15 [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Andrew Pinski
2023-08-11 9:15 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
2023-08-11 9:51 ` Richard Biener
2023-08-11 14:00 ` Jeff Law
2023-08-11 15:07 ` Andrew MacLeod
2023-08-21 21:00 ` Andrew Pinski
2023-09-01 6:40 ` Andrew Pinski
2023-09-07 14:02 ` Andrew MacLeod
2023-08-11 9:50 ` [PATCH 1/2] PHI-OPT [PR 110984]: Add support for NE_EXPR/EQ_EXPR with casts to spaceship_replacement Richard Biener
2023-09-01 17:30 [PATCH 1/2] VR-VALUES: Rename op0/op1 to op1/op2 for test_for_singularity Andrew Pinski
2023-09-01 17:30 ` [PATCH 2/2] VR-VALUES: Rewrite test_for_singularity using range_op_handler Andrew Pinski
2023-09-05 6:06 ` Jeff Law
2023-09-05 7:12 ` Andrew Pinski
2023-09-29 20:17 ` Jeff Law
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).