[PATCH][RFA][P1 PR tree-optimization/83298] Avoid over-optimistic result range for COND_EXPR

[Richard Biener]

On Fri, Dec 8, 2017 at 1:18 AM, Jeff Law <law@redhat.com> wrote:
>
> So the underlying issue here is quite simple.  Given something like
>
> x = (cond) ? res1 : res2;
>
> EVRP analysis will compute the resultant range using vrp_meet of the
> ranges for res1 and res2.  Seems pretty natural.
>
> vrp_meet makes optimistic assumptions if either range is VR_UNDEFINED
> and will set the resultant range to the range of the other operand.
>
> Some callers explicitly mention this is the desired behavior (PHI
> processing).  Other callers avoid calling vrp_meet when one of the
> ranges is VR_UNDEFINED and do something sensible
> (extract_range_from_unary_expr, extract_range_from_binary_expr_1).

Note that "those" simply optimize the VR_UNDEFINED case by ignoring
the range.  They basically take a shortcut around vrp_meet and avoid
calling extract_range_* on VR_UNDEFINED ranges.

> extract_range_from_cond_expr neither mentions that it wants the
> optimistic behavior nor does it avoid calling vrp_meet with a
> VR_UNDEFINED range.  It naturally seems to fit in better with the other
> extract_range_from_* routines.
>
> I'm not at all familiar with the ipa-cp bits, but from a quick look they
> also seems to fall into the extract_* camp.
>
>
> Anyway, normally in a domwalk the only place where we're going to see
> VR_UNDEFINED would be in the PHI nodes.  It's one of the nice properties
> of a domwalk :-)
>
> However, for jump threading we look past the dominance frontier;
> furthermore, we do not currently record ranges for statements we process
> as part of the jump threading.  But we do try to extract the range each
> statement generates -- we're primarily looking for cases where the
> statement generates a singleton range.
>
> While the plan does include recording ranges as we look past the
> dominance frontier, I strongly believe some serious code cleanup in DOM
> and jump threading needs to happen first.  So I don't want to go down
> that path for gcc-8.
>
> So we're kind-of stuck with the fact that we might query for a resultant
> range when one or more input operands may not have recorded range
> information.  Thankfully that's easily resolved by making
> extract_range_from_cond_expr work like the other range extraction
> routines and avoid calling vrp_meet when one or more operands is
> VR_UNDEFINED.
>
> Bootstrapped and regression tested on x86_64.  OK for the trunk?
>
> Jeff
>
>
>         PR tree-optimization/83298
>         * vr-values.c (vr_values::extract_range_from_cond_expr): Do not
>         call vrp_meet if one of the input operands is VR_UNDEFINED.
>
>         PR tree-optimization/83298
>         * gcc.c-torture/execute/pr83298.c: New test.
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr83298.c b/gcc/testsuite/gcc.c-torture/execute/pr83298.c
> new file mode 100644
> index 00000000000..0e51ababf5c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr83298.c
> @@ -0,0 +1,11 @@
> +
> +int a, b, c = 1;
> +
> +int main ()
> +{
> +  for (; b < 1; b++)
> +    ;
> +  if (!(c * (a < 1)))
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/vr-values.c b/gcc/vr-values.c
> index 9352e120d9d..ee5ae3c6a27 100644
> --- a/gcc/vr-values.c
> +++ b/gcc/vr-values.c
> @@ -912,6 +912,23 @@ vr_values::extract_range_from_cond_expr (value_range *vr, gassign *stmt)
>    else
>      set_value_range_to_varying (&vr1);
>
> +  /* If either range is VR_UNDEFINED, vrp_meet will make the optimistic
> +     choice and use the range of the other operand as the result range.
> +
> +     Other users of vrp_meet either explicitly filter the calls for
> +     this case, or they do not care (PHI processing where unexecutable
> +     edges are explicitly expected to be ignored).
> +
> +     Like most other callers, we can not generally tolerate the optimistic
> +     choice here.  Consider jump threading where we're looking into a
> +     non-dominated block and thus may not necessarily have processed the
> +     ranges for statements within that non-dominated block.  */
> +  if (vr0.type == VR_UNDEFINED || vr1.type == VR_UNDEFINED)
> +    {
> +      set_value_range_to_varying (vr);
> +      return;
> +    }
> +
>    /* The resulting value range is the union of the operand ranges */
>    copy_value_range (vr, &vr0);
>    vrp_meet (vr, &vr1);
>