Re: [PR86153] simplify more overflow tests in VRP

[Jeff Law <law at redhat dot com>]

On 12/18/18 3:58 AM, Alexandre Oliva wrote:
> Jeff, you mentioned you had changes to the VRP overflow test that would
> fix this, but I couldn't figure out whether or not you ever put them in
> and it regressed again later, or what.  Anyway, here's my take on it.
No, they're not on the trunk yet.  They're sitting here in my tester --
I lost the testcase I'd written to exercise them and hadn't gone back
and recreated it.

Mine catches fgt32, fge22, fge32, but misses the others in your
testcase.  I was generalizing the code in the same place, targeted
towards the 83239 testcase prior to Jon inserting the
___builtin_unreachable calls into the runtime.  They generalized things
so that instead of +-1 and a comparison against zero, we could have an
arbitrary constant and a relational between A or B and the constant.

I went back and recreated the testcase from 83239 prior to Jon's
patches.  Then verified it will issue a bogus warning on the trunk.
Then I applied your patch to the trunk and verified yours fixes the
warning.  So AFAICT your patch addresses the missed optimization in
83239 as well as the issues in 86153.  Please reference 83239 in your
your ChangeLog and close 83239 when you install  your patch.

I'm going to drop my changes related to 83239.  I don't think they have
much value once your patch is installed, except perhaps to slightly
simplify the code.





> 
> The reason we issued the warnings was that we failed to optimize out
> some parts of _M_fill_insert, used by the C++98 version of vector
> resize, although the call of _M_fill_insert was guarded by a test that
> could never pass: test testcase only calls resize when the vector size
> is >= 3, to decrement the size by two.  The limitation we hit in VRP
> was that the compared values could pass as an overflow test, if the
> vector size was 0 or 1 (we knew it wasn't), but even with dynamic
> ranges we failed to decide that the test result could be determined at
> compile time, even though after the test we introduced ASSERT_EXPRs
> that required a condition known to be false from earlier ones.
> 
> I pondered turning ASSERT_EXPRs that show impossible conditions into
> traps, to enable subsequent instructions to be optimized, but I ended
> up finding an earlier spot in which an overflow test that would have
> introduced the impossible ASSERT_EXPR can have its result deduced from
> earlier known ranges and resolved to the other path.
Right.  IMHO it's better to use the results of the ASSERT_EXPR to deduce
the tighter ranges and either prove a conditional is always true or
always false.


> 
> Although such overflow tests could be uniformly simplified to compares
> against a constant, the original code would only perform such
> simplifications when the test could be resolved to an equality test
> against zero.  I've thus avoided introducing compares against other
> constants, and instead added code that will only simplify overflow
> tests that weren't simplified before when the condition can be
> evaluated at compile time.That limitation was precisely what my (unsubmitted) patch was trying to
address :-)



> 
> 
> Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?
> 
> 
> for  gcc/ChangeLog
> 
> 	PR testsuite/86153
> 	* vr-values.c
> 	(vr_values::vrp_evaluate_conditional_warnv_with_ops): Extend
> 	simplification of overflow tests to cover cases in which we
> 	can determine the result of the comparison.
> 
> for  gcc/testsuite/ChangeLog
> 
> 	PR testsuite/86153
> 	* gcc.dg/vrp-overflow-1.c: New.
> ---
>  gcc/testsuite/gcc.dg/vrp-overflow-1.c |  151 +++++++++++++++++++++++++++++++++
>  gcc/vr-values.c                       |   32 +++++++
>  2 files changed, 183 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/vrp-overflow-1.c
> 

> diff --git a/gcc/vr-values.c b/gcc/vr-values.c
> index cbc759a18e6a..25390ed6ef86 100644
> --- a/gcc/vr-values.c
> +++ b/gcc/vr-values.c
> @@ -2336,6 +2336,38 @@ vr_values::vrp_evaluate_conditional_warnv_with_ops (enum tree_code code,
>  	  op1 = wide_int_to_tree (TREE_TYPE (op0), 0);
>  	  code = (code == GT_EXPR || code == GE_EXPR) ? EQ_EXPR : NE_EXPR;
>  	}
> +      else
> +	{
> +	  value_range vro, vri;
> +	  if (code == GT_EXPR || code == GE_EXPR)
> +	    {
> +	      vro.set (VR_ANTI_RANGE, TYPE_MIN_VALUE (TREE_TYPE (op0)), x);
> +	      vri.set (VR_RANGE, TYPE_MIN_VALUE (TREE_TYPE (op0)), x);
> +	    }
> +	  else if (code == LT_EXPR || code == LE_EXPR)
> +	    {
> +	      vro.set (VR_RANGE, TYPE_MIN_VALUE (TREE_TYPE (op0)), x);
> +	      vri.set (VR_ANTI_RANGE, TYPE_MIN_VALUE (TREE_TYPE (op0)), x);
> +	    }
> +	  else
> +	    gcc_unreachable ();
> +	  value_range *vr0 = get_value_range (op0);
> +	  /* If the range for OP0 to pass the overflow test, namely
> +	     vro, has no intersection with the range for OP0, then the
> +	     overflow test can't pass, so return false.  If it is the
> +	     inverted range, vri, that has no intersection, then the
> +	     overflow test must pass, so return true.  In other cases,
> +	     we could proceed with a simplified condition comparing
> +	     OP0 and X, with LE_EXPR for previously LE_ or LT_EXPR and
> +	     GT_EXPR otherwise, but the comments next ot the enclosing
> +	     if suggest it's not generally profitable to do so.  */
The first sentence doesn't parse well.  s/ot/to in the last sentence.

OK with the comment fixed and addition of PR 83239 to the ChangeLog entry.

jeff