On Fri, Dec 8, 2017 at 1:18 AM, Jeff Law <l...@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).
>
> 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?
But that does regress the case of either arm being an uninitialized
variable.
I'm not convinced that when you look forward past the dominance frontier
and do VRP analysis on stmts without analyzing all intermediate
stmts on the path (or at least push all defs on that path temporarily
to VR_VARYING) is fixed by this patch. It merely looks like a wrong
workaround for a fundamental issue in how DOM now uses the
interface?
Thanks,
Richard.
> 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);
>
