| Message ID | alpine.DEB.2.02.1605132019140.21219@laptop-mg.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
On 05/13/2016 12:50 PM, Marc Glisse wrote: > Hello, > > when VRP does some transforms, it may create new SSA_NAMEs, but doesn't > give them range information. This can prevent cascading transformations > in a single VRP pass. With this patch, I assign range information to the > variable introduced by one transformation, and in another > transformation, I get range information through get_range_info instead > of get_value_range in order to have access to the new information. > > Some notes: > - get_range_info only applies to integers, not pointers. I hope we are > not losing much by restricting this transformation. I could also call > get_value_range and only fall back to get_range_info if that failed (and > we don't have a pointer), but it doesn't seem worth it. It probably isn't worth it. > - Now that I think of it, maybe I should check that the variable is not > a pointer before calling set_range_info? Having range [0, 1] makes it > unlikely, but who knows... Maybe using an assert would be better. > Index: gcc/tree-vrp.c > =================================================================== > --- gcc/tree-vrp.c (revision 236194) > +++ gcc/tree-vrp.c (working copy) > @@ -8933,20 +8933,24 @@ simplify_truth_ops_using_ranges (gimple_ > gimple_assign_set_rhs_with_ops (gsi, > need_conversion > ? NOP_EXPR : TREE_CODE (op0), op0); > /* For A != B we substitute A ^ B. Either with conversion. */ > else if (need_conversion) > { > tree tem = make_ssa_name (TREE_TYPE (op0)); > gassign *newop > = gimple_build_assign (tem, BIT_XOR_EXPR, op0, op1); > gsi_insert_before (gsi, newop, GSI_SAME_STMT); > + if (TYPE_PRECISION (TREE_TYPE (tem)) > 1) > + set_range_info (tem, VR_RANGE, > + wi::zero (TYPE_PRECISION (TREE_TYPE (tem))), > + wi::one (TYPE_PRECISION (TREE_TYPE (tem)))); Is there actually a case where TYPE_PRECISION (TREE_TYPE (tem)) > 1 is ever false? Would an assert make more sense here? > > /* Simplify an integral conversion from an SSA name in STMT. */ > > static bool > simplify_conversion_using_ranges (gimple *stmt) Your ChangeLog mentions simplify_switch_using_ranges, not simplify_conversion_using_ranges. This is OK for the trunk -- your call on asserting the variable is not a pointer before calling set_range_info. Similarly on the check that the TYPE_PRECISION (TREE_TYPE (tem)) > 1. Jeff
On Mon, 16 May 2016, Jeff Law wrote: >> - Now that I think of it, maybe I should check that the variable is not >> a pointer before calling set_range_info? Having range [0, 1] makes it >> unlikely, but who knows... > Maybe using an assert would be better. I don't think having a pointer there would be completely wrong, just unlikely, so I'd rather add a check but not assert. >> Index: gcc/tree-vrp.c >> =================================================================== >> --- gcc/tree-vrp.c (revision 236194) >> +++ gcc/tree-vrp.c (working copy) >> @@ -8933,20 +8933,24 @@ simplify_truth_ops_using_ranges (gimple_ >> gimple_assign_set_rhs_with_ops (gsi, >> need_conversion >> ? NOP_EXPR : TREE_CODE (op0), op0); >> /* For A != B we substitute A ^ B. Either with conversion. */ >> else if (need_conversion) >> { >> tree tem = make_ssa_name (TREE_TYPE (op0)); >> gassign *newop >> = gimple_build_assign (tem, BIT_XOR_EXPR, op0, op1); >> gsi_insert_before (gsi, newop, GSI_SAME_STMT); >> + if (TYPE_PRECISION (TREE_TYPE (tem)) > 1) >> + set_range_info (tem, VR_RANGE, >> + wi::zero (TYPE_PRECISION (TREE_TYPE (tem))), >> + wi::one (TYPE_PRECISION (TREE_TYPE (tem)))); > Is there actually a case where TYPE_PRECISION (TREE_TYPE (tem)) > 1 is ever > false? Would an assert make more sense here? op0 can have precision 1, so tem can as well. In most cases I would expect need_conversion to be false in that case though. However, it doesn't seem impossible to have several types with 1-bit precision that are not equivalent (different TYPE_SIGN for instance). So again, I don't feel comfortable adding an assert. But I am open to proofs that those events cannot happen. >> static bool >> simplify_conversion_using_ranges (gimple *stmt) > Your ChangeLog mentions simplify_switch_using_ranges, not > simplify_conversion_using_ranges. Oups, bad copy-paste (I keep too much context in the diff for diff -p to give useful results), thanks. > This is OK for the trunk -- your call on asserting the variable is not a > pointer before calling set_range_info. Similarly on the check that the > TYPE_PRECISION (TREE_TYPE (tem)) > 1.
Index: gcc/testsuite/gcc.dg/tree-ssa/pr69270.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/pr69270.c (revision 236194) +++ gcc/testsuite/gcc.dg/tree-ssa/pr69270.c (working copy) @@ -1,21 +1,19 @@ /* { dg-do compile } */ /* { dg-options "-O2 -fsplit-paths -fdump-tree-dom3-details" } */ /* There should be two references to bufferstep that turn into constants. */ /* { dg-final { scan-tree-dump-times "Replaced .bufferstep_\[0-9\]+. with constant .0." 1 "dom3"} } */ /* { dg-final { scan-tree-dump-times "Replaced .bufferstep_\[0-9\]+. with constant .1." 1 "dom3"} } */ /* And some assignments ought to fold down to constants. */ -/* { dg-final { scan-tree-dump-times "Folded to: _\[0-9\]+ = -1;" 1 "dom3"} } */ -/* { dg-final { scan-tree-dump-times "Folded to: _\[0-9\]+ = -2;" 1 "dom3"} } */ /* { dg-final { scan-tree-dump-times "Folded to: _\[0-9\]+ = 1;" 1 "dom3"} } */ /* { dg-final { scan-tree-dump-times "Folded to: _\[0-9\]+ = 0;" 1 "dom3"} } */ /* The XOR operations should have been optimized to constants. */ /* { dg-final { scan-tree-dump-not "bit_xor" "dom3"} } */ extern int *stepsizeTable; void Index: gcc/testsuite/gcc.dg/tree-ssa/vrp99.c =================================================================== --- gcc/testsuite/gcc.dg/tree-ssa/vrp99.c (revision 0) +++ gcc/testsuite/gcc.dg/tree-ssa/vrp99.c (working copy) @@ -0,0 +1,9 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-vrp1" } */ + +unsigned f(unsigned i){ + i >>= __SIZEOF_INT__ * __CHAR_BIT__ - 1; + return i == 0; +} + +/* { dg-final { scan-tree-dump-not "\\(unsigned int\\)" "vrp1" } } */ Index: gcc/tree-vrp.c =================================================================== --- gcc/tree-vrp.c (revision 236194) +++ gcc/tree-vrp.c (working copy) @@ -8933,20 +8933,24 @@ simplify_truth_ops_using_ranges (gimple_ gimple_assign_set_rhs_with_ops (gsi, need_conversion ? NOP_EXPR : TREE_CODE (op0), op0); /* For A != B we substitute A ^ B. Either with conversion. */ else if (need_conversion) { tree tem = make_ssa_name (TREE_TYPE (op0)); gassign *newop = gimple_build_assign (tem, BIT_XOR_EXPR, op0, op1); gsi_insert_before (gsi, newop, GSI_SAME_STMT); + if (TYPE_PRECISION (TREE_TYPE (tem)) > 1) + set_range_info (tem, VR_RANGE, + wi::zero (TYPE_PRECISION (TREE_TYPE (tem))), + wi::one (TYPE_PRECISION (TREE_TYPE (tem)))); gimple_assign_set_rhs_with_ops (gsi, NOP_EXPR, tem); } /* Or without. */ else gimple_assign_set_rhs_with_ops (gsi, BIT_XOR_EXPR, op0, op1); update_stmt (gsi_stmt (*gsi)); return true; } @@ -9641,50 +9645,48 @@ simplify_switch_using_ranges (gswitch *s return false; } /* Simplify an integral conversion from an SSA name in STMT. */ static bool simplify_conversion_using_ranges (gimple *stmt) { tree innerop, middleop, finaltype; gimple *def_stmt; - value_range *innervr; signop inner_sgn, middle_sgn, final_sgn; unsigned inner_prec, middle_prec, final_prec; widest_int innermin, innermed, innermax, middlemin, middlemed, middlemax; finaltype = TREE_TYPE (gimple_assign_lhs (stmt)); if (!INTEGRAL_TYPE_P (finaltype)) return false; middleop = gimple_assign_rhs1 (stmt); def_stmt = SSA_NAME_DEF_STMT (middleop); if (!is_gimple_assign (def_stmt) || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt))) return false; innerop = gimple_assign_rhs1 (def_stmt); if (TREE_CODE (innerop) != SSA_NAME || SSA_NAME_OCCURS_IN_ABNORMAL_PHI (innerop)) return false; - /* Get the value-range of the inner operand. */ - innervr = get_value_range (innerop); - if (innervr->type != VR_RANGE - || TREE_CODE (innervr->min) != INTEGER_CST - || TREE_CODE (innervr->max) != INTEGER_CST) + /* Get the value-range of the inner operand. Use get_range_info in + case innerop was created during substitute-and-fold. */ + wide_int imin, imax; + if (!INTEGRAL_TYPE_P (TREE_TYPE (innerop)) + || get_range_info (innerop, &imin, &imax) != VR_RANGE) return false; + innermin = widest_int::from (imin, TYPE_SIGN (TREE_TYPE (innerop))); + innermax = widest_int::from (imax, TYPE_SIGN (TREE_TYPE (innerop))); /* Simulate the conversion chain to check if the result is equal if the middle conversion is removed. */ - innermin = wi::to_widest (innervr->min); - innermax = wi::to_widest (innervr->max); - inner_prec = TYPE_PRECISION (TREE_TYPE (innerop)); middle_prec = TYPE_PRECISION (TREE_TYPE (middleop)); final_prec = TYPE_PRECISION (finaltype); /* If the first conversion is not injective, the second must not be widening. */ if (wi::gtu_p (innermax - innermin, wi::mask <widest_int> (middle_prec, false)) && middle_prec < final_prec) return false;