Re: undefined behavior in value_range::equiv_add()?
On 6/6/19 4:11 PM, Aldy Hernandez wrote: >> Meanwhile I have bootstrapped / tested the following which does the VARYING >> >> thing. >> >> Applied to trunk. I think we need to backport this since this is a latent >> >> wrong-code issue. We can see to improve things on the trunk incrementally. >> > > Folks, thanks so much for taking care of this. > > After Richard's patch, my value_range_base::intersect patch no longer > fails on vrp47, and no longer requires a special-case for undefined. > > The attached patch splits out the intersect code into a value_range_base > version, as we have for union_. > > OK? > > Aldy > > curr.patch > > gcc/ > > * tree-vrp.h (value_range_base::intersect): New. > (value_range::intersect_helper): Move from here... > (value_range_base::intersect_helper): ...to here. > * tree-vrp.c (value_range::intersect_helper): Rename to... > (value_range_base::intersect_helper): ...this, and rewrite to > return a value instead of modifying THIS in place. > Also, move equivalence handling... > (value_range::intersect): ...here, while calling intersect_helper. > * gimple-fold.c (size_must_be_zero_p): Use value_range_base when > calling intersect. > * gimple-ssa-evrp-analyze.c (ecord_ranges_from_incoming_edge): > Same. > * vr-values.c (vrp_evaluate_conditional_warnv_with_ops): Same. OK jeff
Re: undefined behavior in value_range::equiv_add()?
Meanwhile I have bootstrapped / tested the following which does the VARYING
thing.
Applied to trunk. I think we need to backport this since this is a latent
wrong-code issue. We can see to improve things on the trunk incrementally.
Folks, thanks so much for taking care of this.
After Richard's patch, my value_range_base::intersect patch no longer
fails on vrp47, and no longer requires a special-case for undefined.
The attached patch splits out the intersect code into a value_range_base
version, as we have for union_.
OK?
Aldy
gcc/
* tree-vrp.h (value_range_base::intersect): New.
(value_range::intersect_helper): Move from here...
(value_range_base::intersect_helper): ...to here.
* tree-vrp.c (value_range::intersect_helper): Rename to...
(value_range_base::intersect_helper): ...this, and rewrite to
return a value instead of modifying THIS in place.
Also, move equivalence handling...
(value_range::intersect): ...here, while calling intersect_helper.
* gimple-fold.c (size_must_be_zero_p): Use value_range_base when
calling intersect.
* gimple-ssa-evrp-analyze.c (ecord_ranges_from_incoming_edge):
Same.
* vr-values.c (vrp_evaluate_conditional_warnv_with_ops): Same.
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index b3e931744f8..8b8331eb555 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -684,10 +684,10 @@ size_must_be_zero_p (tree size)
/* Compute the value of SSIZE_MAX, the largest positive value that
can be stored in ssize_t, the signed counterpart of size_t. */
wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
- value_range valid_range (VR_RANGE,
- build_int_cst (type, 0),
- wide_int_to_tree (type, ssize_max));
- value_range vr;
+ value_range_base valid_range (VR_RANGE,
+build_int_cst (type, 0),
+wide_int_to_tree (type, ssize_max));
+ value_range_base vr;
get_range_info (size, vr);
vr.intersect (&valid_range);
return vr.zero_p ();
diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index bb4e2d6e798..4c68af847e1 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -210,9 +210,10 @@ evrp_range_analyzer::record_ranges_from_incoming_edge (basic_block bb)
getting first [64, +INF] and then ~[0, 0] from
conditions like (s & 0x3cc0) == 0). */
value_range *old_vr = get_value_range (vrs[i].first);
- value_range tem (old_vr->kind (), old_vr->min (), old_vr->max ());
+ value_range_base tem (old_vr->kind (), old_vr->min (),
+old_vr->max ());
tem.intersect (vrs[i].second);
- if (tem.equal_p (*old_vr, /*ignore_equivs=*/true))
+ if (tem.equal_p (*old_vr))
continue;
push_value_range (vrs[i].first, vrs[i].second);
if (is_fallthru
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index fdda64c30d5..d94de2b22ee 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -6020,30 +6020,26 @@ intersect_ranges (enum value_range_kind *vr0type,
}
-/* Intersect the two value-ranges *VR0 and *VR1 and store the result
- in *VR0. This may not be the smallest possible such range. */
+/* Helper for the intersection operation for value ranges. Given two
+ value ranges VR0 and VR1, return the intersection of the two
+ ranges. This may not be the smallest possible such range. */
-void
-value_range::intersect_helper (value_range *vr0, const value_range *vr1)
+value_range_base
+value_range_base::intersect_helper (const value_range_base *vr0,
+const value_range_base *vr1)
{
/* If either range is VR_VARYING the other one wins. */
if (vr1->varying_p ())
-return;
+return *vr0;
if (vr0->varying_p ())
-{
- vr0->deep_copy (vr1);
- return;
-}
+return *vr1;
/* When either range is VR_UNDEFINED the resulting range is
VR_UNDEFINED, too. */
if (vr0->undefined_p ())
-return;
+return *vr0;
if (vr1->undefined_p ())
-{
- vr0->set_undefined ();
- return;
-}
+return *vr1;
value_range_kind vr0type = vr0->kind ();
tree vr0min = vr0->min ();
@@ -6053,28 +6049,34 @@ value_range::intersect_helper (value_range *vr0, const value_range *vr1)
/* Make sure to canonicalize the result though as the inversion of a
VR_RANGE can still be a VR_RANGE. Work on a temporary so we can
fall back to vr0 when this turns things to varying. */
- value_range tem;
+ value_range_base tem;
tem.set_and_canonicalize (vr0type, vr0min, vr0max);
/* If that failed, use the saved original VR0. */
if (tem.varying_p ())
-return;
- vr0->update (tem.kind (), tem.min (), tem.max ());
+return *vr0;
- /* If the result is VR_UNDEFINED there is no need to mess with
- the equivalencies. */
- if (vr0->undefined_p ())
-return;
+ return tem;
+}
- /* The resulting set of equivalences for range intersection is the union of
- the two sets. */
- if (vr0->m_equiv && vr1->m_equiv && vr0->m_equiv != vr1->m_equiv)
-
Re: undefined behavior in value_range::equiv_add()?
On 6/6/19 1:31 AM, Richard Biener wrote: [ Big snip ] > >> I was primarily concerned with the ones in the evrp simplification >> engine which are easy to fix in-place. Looking at gimple-fold.c I agree >> we do need to address the walking problem. >> >> I'm not sure we can drop to varying though -- my first twiddle of this >> did precisely that, but that'll likely regress vrp47 where we know the >> resultant range after simplification is just [0,1]. > > Of course we do not need to drop the original LHS to VARYING, only > the defs we didn't already visit. I was referring to the newly created def. There's a case (seen in vrp47 IIRC) where the new temporary has a known range [0,1] and we need to know that for subsequent simplifications. But if your change passes regression testing, then we're still picking that up properly. > >> Ideally we wouldn't have the simplifiers creating new names or we'd have >> a more robust mechanism for identifying when we've created a new name >> and doing the right thing. I suspect whatever we do here right now is >> going to be a bandaid and as long as we keep creating new names in the >> simplifier we're likely going to be coming back and applying more bandaids. > > Re-visiting the stmts would be possible if we'd split registering ranges > derived > from uses of a stmt from those of defs (IIRC I had incomplete patches to do > that > when trying to derive X != 0 from Y / X because otherwise we miscompile > stuff). > > Meanwhile I have bootstrapped / tested the following which does the VARYING > thing. > > Applied to trunk. I think we need to backport this since this is a latent > wrong-code issue. We can see to improve things on the trunk incrementally. Works for me. jeff
Re: undefined behavior in value_range::equiv_add()?
On Wed, Jun 5, 2019 at 11:12 PM Jeff Law wrote:
>
> On 6/4/19 9:04 AM, Richard Biener wrote:
> > On Tue, Jun 4, 2019 at 3:40 PM Jeff Law wrote:
> >>
> >> On 6/4/19 5:23 AM, Richard Biener wrote:
> >>> On Tue, Jun 4, 2019 at 12:30 AM Jeff Law wrote:
>
> On 6/3/19 7:13 AM, Aldy Hernandez wrote:
> > On 5/31/19 5:00 AM, Richard Biener wrote:
> >> On Fri, May 31, 2019 at 2:27 AM Jeff Law
> >> wrote:
> >>>
> >>> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> On 5/29/19 12:12 PM, Jeff Law wrote:
> > On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> >> On 5/29/19 9:24 AM, Richard Biener wrote:
> >>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
> >>> wrote:
>
> As per the API, and the original documentation
> to value_range, VR_UNDEFINED and VR_VARYING
> should never have equivalences. However,
> equiv_add is tacking on equivalences blindly,
> and there are various regressions that happen
> if I fix this oversight.
>
> void value_range::equiv_add (const_tree var,
> const value_range *var_vr, bitmap_obstack
> *obstack) { if (!m_equiv) m_equiv =
> BITMAP_ALLOC (obstack); unsigned ver =
> SSA_NAME_VERSION (var); bitmap_set_bit
> (m_equiv, ver); if (var_vr && var_vr->m_equiv)
> bitmap_ior_into (m_equiv, var_vr->m_equiv); }
>
> Is this a bug in the documentation / API, or is
> equiv_add incorrect and we should fix the
> fall-out elsewhere?
> >>>
> >>> I think this must have been crept in during the
> >>> classification. If you go back to say GCC 7 you
> >>> shouldn't see value-ranges with UNDEFINED/VARYING
> >>> state in the lattice that have equivalences.
> >>>
> >>> It may not be easy to avoid with the new classy
> >>> interface but we're certainly not tacking on them
> >>> "blindly". At least we're not supposed to. As
> >>> usual the intermediate state might be "broken"
> >>> but intermediateness is not sth the new class
> >>> "likes".
> >>
> >> It looks like extract_range_from_stmt (by virtue
> >> of vrp_visit_assignment_or_call and then
> >> extract_range_from_ssa_name) returns one of these
> >> intermediate ranges. It would seem to me that an
> >> outward looking API method like
> >> vr_values::extract_range_from_stmt shouldn't be
> >> returning inconsistent ranges. Or are there no
> >> guarantees for value_ranges from within all of
> >> vr_values?
> > ISTM that if we have an implementation constraint
> > that says a VR_VARYING or VR_UNDEFINED range can't
> > have equivalences, then we need to honor that at the
> > minimum for anything returned by an external API.
> > Returning an inconsistent state is bad. I'd even
> > state that we should try damn hard to avoid it in
> > internal APIs as well.
>
> Agreed * 2.
>
> >
> >>
> >> Perhaps I should give a little background. As part
> >> of your value_range_base re-factoring last year,
> >> you mentioned that you didn't split out intersect
> >> like you did union because of time or oversight.
> >> I have code to implement intersect (attached), for
> >> which I've noticed that I must leave equivalences
> >> intact, even when transitioning to VR_UNDEFINED:
> >>
> >> [from the attached patch] + /* If THIS is varying
> >> we want to pick up equivalences from OTHER. +
> >> Just special-case this here rather than trying to
> >> fixup after the + fact. */ + if
> >> (this->varying_p ()) +this->deep_copy (other);
> >> + else if (this->undefined_p ()) +/* ?? Leave
> >> any equivalences already present in an undefined. +
> >> This is technically not allowed, but we may get an
> >> in-flight + value_range in an intermediate
> >> state. */
> > Where/when does this happen?
>
> The above snippet is not currently in mainline. It's
> in the patch I'm proposing to clean up intersect. It's
> just that while cleaning up intersect I noticed that if
> we keep to the value_range API, we end up clobbering an
> equivalence to a VR_UNDEFINED that we depend up further
> up the call chain.
>
> The reason it doesn't happen in mainline is because
> intersect_helper bails early on an undefined, thus
> leaving the problematic equivalence intact.
>
> You can see it in mainline though, with the following
> testcase:
> >
Re: undefined behavior in value_range::equiv_add()?
On 6/4/19 9:04 AM, Richard Biener wrote:
> On Tue, Jun 4, 2019 at 3:40 PM Jeff Law wrote:
>>
>> On 6/4/19 5:23 AM, Richard Biener wrote:
>>> On Tue, Jun 4, 2019 at 12:30 AM Jeff Law wrote:
On 6/3/19 7:13 AM, Aldy Hernandez wrote:
> On 5/31/19 5:00 AM, Richard Biener wrote:
>> On Fri, May 31, 2019 at 2:27 AM Jeff Law
>> wrote:
>>>
>>> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
On 5/29/19 12:12 PM, Jeff Law wrote:
> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
>> On 5/29/19 9:24 AM, Richard Biener wrote:
>>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
>>> wrote:
As per the API, and the original documentation
to value_range, VR_UNDEFINED and VR_VARYING
should never have equivalences. However,
equiv_add is tacking on equivalences blindly,
and there are various regressions that happen
if I fix this oversight.
void value_range::equiv_add (const_tree var,
const value_range *var_vr, bitmap_obstack
*obstack) { if (!m_equiv) m_equiv =
BITMAP_ALLOC (obstack); unsigned ver =
SSA_NAME_VERSION (var); bitmap_set_bit
(m_equiv, ver); if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv); }
Is this a bug in the documentation / API, or is
equiv_add incorrect and we should fix the
fall-out elsewhere?
>>>
>>> I think this must have been crept in during the
>>> classification. If you go back to say GCC 7 you
>>> shouldn't see value-ranges with UNDEFINED/VARYING
>>> state in the lattice that have equivalences.
>>>
>>> It may not be easy to avoid with the new classy
>>> interface but we're certainly not tacking on them
>>> "blindly". At least we're not supposed to. As
>>> usual the intermediate state might be "broken"
>>> but intermediateness is not sth the new class
>>> "likes".
>>
>> It looks like extract_range_from_stmt (by virtue
>> of vrp_visit_assignment_or_call and then
>> extract_range_from_ssa_name) returns one of these
>> intermediate ranges. It would seem to me that an
>> outward looking API method like
>> vr_values::extract_range_from_stmt shouldn't be
>> returning inconsistent ranges. Or are there no
>> guarantees for value_ranges from within all of
>> vr_values?
> ISTM that if we have an implementation constraint
> that says a VR_VARYING or VR_UNDEFINED range can't
> have equivalences, then we need to honor that at the
> minimum for anything returned by an external API.
> Returning an inconsistent state is bad. I'd even
> state that we should try damn hard to avoid it in
> internal APIs as well.
Agreed * 2.
>
>>
>> Perhaps I should give a little background. As part
>> of your value_range_base re-factoring last year,
>> you mentioned that you didn't split out intersect
>> like you did union because of time or oversight.
>> I have code to implement intersect (attached), for
>> which I've noticed that I must leave equivalences
>> intact, even when transitioning to VR_UNDEFINED:
>>
>> [from the attached patch] + /* If THIS is varying
>> we want to pick up equivalences from OTHER. +
>> Just special-case this here rather than trying to
>> fixup after the + fact. */ + if
>> (this->varying_p ()) +this->deep_copy (other);
>> + else if (this->undefined_p ()) +/* ?? Leave
>> any equivalences already present in an undefined. +
>> This is technically not allowed, but we may get an
>> in-flight + value_range in an intermediate
>> state. */
> Where/when does this happen?
The above snippet is not currently in mainline. It's
in the patch I'm proposing to clean up intersect. It's
just that while cleaning up intersect I noticed that if
we keep to the value_range API, we end up clobbering an
equivalence to a VR_UNDEFINED that we depend up further
up the call chain.
The reason it doesn't happen in mainline is because
intersect_helper bails early on an undefined, thus
leaving the problematic equivalence intact.
You can see it in mainline though, with the following
testcase:
int f(int x) { if (x != 0 && x != 1) return -2;
return !x; }
Break in evrp_range_analyzer::record_ranges_from_stmt()
and see that the call to extract_range_from_stmt()
>>
Re: undefined behavior in value_range::equiv_add()?
On Tue, Jun 4, 2019 at 3:40 PM Jeff Law wrote:
>
> On 6/4/19 5:23 AM, Richard Biener wrote:
> > On Tue, Jun 4, 2019 at 12:30 AM Jeff Law wrote:
> >>
> >> On 6/3/19 7:13 AM, Aldy Hernandez wrote:
> >>> On 5/31/19 5:00 AM, Richard Biener wrote:
> On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
> >
> > On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> >> On 5/29/19 12:12 PM, Jeff Law wrote:
> >>> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> On 5/29/19 9:24 AM, Richard Biener wrote:
> > On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
> > wrote:
> >>
> >> As per the API, and the original documentation to value_range,
> >> VR_UNDEFINED and VR_VARYING should never have equivalences.
> >> However,
> >> equiv_add is tacking on equivalences blindly, and there are various
> >> regressions that happen if I fix this oversight.
> >>
> >> void
> >> value_range::equiv_add (const_tree var,
> >>const value_range *var_vr,
> >>bitmap_obstack *obstack)
> >> {
> >> if (!m_equiv)
> >> m_equiv = BITMAP_ALLOC (obstack);
> >> unsigned ver = SSA_NAME_VERSION (var);
> >> bitmap_set_bit (m_equiv, ver);
> >> if (var_vr && var_vr->m_equiv)
> >> bitmap_ior_into (m_equiv, var_vr->m_equiv);
> >> }
> >>
> >> Is this a bug in the documentation / API, or is equiv_add incorrect
> >> and
> >> we should fix the fall-out elsewhere?
> >
> > I think this must have been crept in during the classification.
> > If you
> > go back to say GCC 7 you shouldn't see value-ranges with
> > UNDEFINED/VARYING state in the lattice that have equivalences.
> >
> > It may not be easy to avoid with the new classy interface but we're
> > certainly not tacking on them "blindly". At least we're not
> > supposed
> > to. As usual the intermediate state might be "broken" but
> > intermediateness is not sth the new class "likes".
>
> It looks like extract_range_from_stmt (by virtue of
> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
> returns one of these intermediate ranges. It would seem to me
> that an
> outward looking API method like vr_values::extract_range_from_stmt
> shouldn't be returning inconsistent ranges. Or are there no
> guarantees
> for value_ranges from within all of vr_values?
> >>> ISTM that if we have an implementation constraint that says a
> >>> VR_VARYING
> >>> or VR_UNDEFINED range can't have equivalences, then we need to honor
> >>> that at the minimum for anything returned by an external API.
> >>> Returning
> >>> an inconsistent state is bad. I'd even state that we should try damn
> >>> hard to avoid it in internal APIs as well.
> >>
> >> Agreed * 2.
> >>
> >>>
>
> Perhaps I should give a little background. As part of your
> value_range_base re-factoring last year, you mentioned that you
> didn't
> split out intersect like you did union because of time or
> oversight. I
> have code to implement intersect (attached), for which I've
> noticed that
> I must leave equivalences intact, even when transitioning to
> VR_UNDEFINED:
>
> [from the attached patch]
> + /* If THIS is varying we want to pick up equivalences from OTHER.
> + Just special-case this here rather than trying to fixup
> after the
> + fact. */
> + if (this->varying_p ())
> +this->deep_copy (other);
> + else if (this->undefined_p ())
> +/* ?? Leave any equivalences already present in an undefined.
> + This is technically not allowed, but we may get an in-flight
> + value_range in an intermediate state. */
> >>> Where/when does this happen?
> >>
> >> The above snippet is not currently in mainline. It's in the patch I'm
> >> proposing to clean up intersect. It's just that while cleaning up
> >> intersect I noticed that if we keep to the value_range API, we end up
> >> clobbering an equivalence to a VR_UNDEFINED that we depend up
> >> further up
> >> the call chain.
> >>
> >> The reason it doesn't happen in mainline is because intersect_helper
> >> bails early on an undefined, thus leaving the problematic equivalence
> >> intact.
> >>
> >> You can see it in mainline though, with the following testcase:
> >>
> >> int f(int x)
> >> {
> >>if (x != 0 && x != 1)
> >> return -2;
> >>
> >>return !x;
> >> }
> >>
>
Re: undefined behavior in value_range::equiv_add()?
On 6/4/19 5:23 AM, Richard Biener wrote:
> On Tue, Jun 4, 2019 at 12:30 AM Jeff Law wrote:
>>
>> On 6/3/19 7:13 AM, Aldy Hernandez wrote:
>>> On 5/31/19 5:00 AM, Richard Biener wrote:
On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
>
> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
>> On 5/29/19 12:12 PM, Jeff Law wrote:
>>> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
On 5/29/19 9:24 AM, Richard Biener wrote:
> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
> wrote:
>>
>> As per the API, and the original documentation to value_range,
>> VR_UNDEFINED and VR_VARYING should never have equivalences.
>> However,
>> equiv_add is tacking on equivalences blindly, and there are various
>> regressions that happen if I fix this oversight.
>>
>> void
>> value_range::equiv_add (const_tree var,
>>const value_range *var_vr,
>>bitmap_obstack *obstack)
>> {
>> if (!m_equiv)
>> m_equiv = BITMAP_ALLOC (obstack);
>> unsigned ver = SSA_NAME_VERSION (var);
>> bitmap_set_bit (m_equiv, ver);
>> if (var_vr && var_vr->m_equiv)
>> bitmap_ior_into (m_equiv, var_vr->m_equiv);
>> }
>>
>> Is this a bug in the documentation / API, or is equiv_add incorrect
>> and
>> we should fix the fall-out elsewhere?
>
> I think this must have been crept in during the classification.
> If you
> go back to say GCC 7 you shouldn't see value-ranges with
> UNDEFINED/VARYING state in the lattice that have equivalences.
>
> It may not be easy to avoid with the new classy interface but we're
> certainly not tacking on them "blindly". At least we're not
> supposed
> to. As usual the intermediate state might be "broken" but
> intermediateness is not sth the new class "likes".
It looks like extract_range_from_stmt (by virtue of
vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
returns one of these intermediate ranges. It would seem to me
that an
outward looking API method like vr_values::extract_range_from_stmt
shouldn't be returning inconsistent ranges. Or are there no
guarantees
for value_ranges from within all of vr_values?
>>> ISTM that if we have an implementation constraint that says a
>>> VR_VARYING
>>> or VR_UNDEFINED range can't have equivalences, then we need to honor
>>> that at the minimum for anything returned by an external API.
>>> Returning
>>> an inconsistent state is bad. I'd even state that we should try damn
>>> hard to avoid it in internal APIs as well.
>>
>> Agreed * 2.
>>
>>>
Perhaps I should give a little background. As part of your
value_range_base re-factoring last year, you mentioned that you
didn't
split out intersect like you did union because of time or
oversight. I
have code to implement intersect (attached), for which I've
noticed that
I must leave equivalences intact, even when transitioning to
VR_UNDEFINED:
[from the attached patch]
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup
after the
+ fact. */
+ if (this->varying_p ())
+this->deep_copy (other);
+ else if (this->undefined_p ())
+/* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
>>> Where/when does this happen?
>>
>> The above snippet is not currently in mainline. It's in the patch I'm
>> proposing to clean up intersect. It's just that while cleaning up
>> intersect I noticed that if we keep to the value_range API, we end up
>> clobbering an equivalence to a VR_UNDEFINED that we depend up
>> further up
>> the call chain.
>>
>> The reason it doesn't happen in mainline is because intersect_helper
>> bails early on an undefined, thus leaving the problematic equivalence
>> intact.
>>
>> You can see it in mainline though, with the following testcase:
>>
>> int f(int x)
>> {
>>if (x != 0 && x != 1)
>> return -2;
>>
>>return !x;
>> }
>>
>> Break in evrp_range_analyzer::record_ranges_from_stmt() and see that
>> the
>> call to extract_range_from_stmt() returns a VR of undefined *WITH*
>> equivalences:
>>
>>vr_values->extract_range_from_stmt (stmt, &taken_edge,
>> &output, &
Re: undefined behavior in value_range::equiv_add()?
On Tue, Jun 4, 2019 at 12:30 AM Jeff Law wrote:
>
> On 6/3/19 7:13 AM, Aldy Hernandez wrote:
> > On 5/31/19 5:00 AM, Richard Biener wrote:
> >> On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
> >>>
> >>> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> On 5/29/19 12:12 PM, Jeff Law wrote:
> > On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> >> On 5/29/19 9:24 AM, Richard Biener wrote:
> >>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
> >>> wrote:
>
> As per the API, and the original documentation to value_range,
> VR_UNDEFINED and VR_VARYING should never have equivalences.
> However,
> equiv_add is tacking on equivalences blindly, and there are various
> regressions that happen if I fix this oversight.
>
> void
> value_range::equiv_add (const_tree var,
> const value_range *var_vr,
> bitmap_obstack *obstack)
> {
> if (!m_equiv)
> m_equiv = BITMAP_ALLOC (obstack);
> unsigned ver = SSA_NAME_VERSION (var);
> bitmap_set_bit (m_equiv, ver);
> if (var_vr && var_vr->m_equiv)
> bitmap_ior_into (m_equiv, var_vr->m_equiv);
> }
>
> Is this a bug in the documentation / API, or is equiv_add incorrect
> and
> we should fix the fall-out elsewhere?
> >>>
> >>> I think this must have been crept in during the classification.
> >>> If you
> >>> go back to say GCC 7 you shouldn't see value-ranges with
> >>> UNDEFINED/VARYING state in the lattice that have equivalences.
> >>>
> >>> It may not be easy to avoid with the new classy interface but we're
> >>> certainly not tacking on them "blindly". At least we're not
> >>> supposed
> >>> to. As usual the intermediate state might be "broken" but
> >>> intermediateness is not sth the new class "likes".
> >>
> >> It looks like extract_range_from_stmt (by virtue of
> >> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
> >> returns one of these intermediate ranges. It would seem to me
> >> that an
> >> outward looking API method like vr_values::extract_range_from_stmt
> >> shouldn't be returning inconsistent ranges. Or are there no
> >> guarantees
> >> for value_ranges from within all of vr_values?
> > ISTM that if we have an implementation constraint that says a
> > VR_VARYING
> > or VR_UNDEFINED range can't have equivalences, then we need to honor
> > that at the minimum for anything returned by an external API.
> > Returning
> > an inconsistent state is bad. I'd even state that we should try damn
> > hard to avoid it in internal APIs as well.
>
> Agreed * 2.
>
> >
> >>
> >> Perhaps I should give a little background. As part of your
> >> value_range_base re-factoring last year, you mentioned that you
> >> didn't
> >> split out intersect like you did union because of time or
> >> oversight. I
> >> have code to implement intersect (attached), for which I've
> >> noticed that
> >> I must leave equivalences intact, even when transitioning to
> >> VR_UNDEFINED:
> >>
> >> [from the attached patch]
> >> + /* If THIS is varying we want to pick up equivalences from OTHER.
> >> + Just special-case this here rather than trying to fixup
> >> after the
> >> + fact. */
> >> + if (this->varying_p ())
> >> +this->deep_copy (other);
> >> + else if (this->undefined_p ())
> >> +/* ?? Leave any equivalences already present in an undefined.
> >> + This is technically not allowed, but we may get an in-flight
> >> + value_range in an intermediate state. */
> > Where/when does this happen?
>
> The above snippet is not currently in mainline. It's in the patch I'm
> proposing to clean up intersect. It's just that while cleaning up
> intersect I noticed that if we keep to the value_range API, we end up
> clobbering an equivalence to a VR_UNDEFINED that we depend up
> further up
> the call chain.
>
> The reason it doesn't happen in mainline is because intersect_helper
> bails early on an undefined, thus leaving the problematic equivalence
> intact.
>
> You can see it in mainline though, with the following testcase:
>
> int f(int x)
> {
> if (x != 0 && x != 1)
> return -2;
>
> return !x;
> }
>
> Break in evrp_range_analyzer::record_ranges_from_stmt() and see that
> the
> call to extract_range_from_stmt() returns a VR of undefined *WITH*
> equivalences:
>
> vr_values->extract_range_from_stmt (stmt, &taken_edge,
> &output, &vr);
>
> This VR is later fed to up
Re: undefined behavior in value_range::equiv_add()?
On 6/3/19 7:13 AM, Aldy Hernandez wrote:
> On 5/31/19 5:00 AM, Richard Biener wrote:
>> On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
>>>
>>> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
On 5/29/19 12:12 PM, Jeff Law wrote:
> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
>> On 5/29/19 9:24 AM, Richard Biener wrote:
>>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
>>> wrote:
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences.
However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect
and
we should fix the fall-out elsewhere?
>>>
>>> I think this must have been crept in during the classification.
>>> If you
>>> go back to say GCC 7 you shouldn't see value-ranges with
>>> UNDEFINED/VARYING state in the lattice that have equivalences.
>>>
>>> It may not be easy to avoid with the new classy interface but we're
>>> certainly not tacking on them "blindly". At least we're not
>>> supposed
>>> to. As usual the intermediate state might be "broken" but
>>> intermediateness is not sth the new class "likes".
>>
>> It looks like extract_range_from_stmt (by virtue of
>> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
>> returns one of these intermediate ranges. It would seem to me
>> that an
>> outward looking API method like vr_values::extract_range_from_stmt
>> shouldn't be returning inconsistent ranges. Or are there no
>> guarantees
>> for value_ranges from within all of vr_values?
> ISTM that if we have an implementation constraint that says a
> VR_VARYING
> or VR_UNDEFINED range can't have equivalences, then we need to honor
> that at the minimum for anything returned by an external API.
> Returning
> an inconsistent state is bad. I'd even state that we should try damn
> hard to avoid it in internal APIs as well.
Agreed * 2.
>
>>
>> Perhaps I should give a little background. As part of your
>> value_range_base re-factoring last year, you mentioned that you
>> didn't
>> split out intersect like you did union because of time or
>> oversight. I
>> have code to implement intersect (attached), for which I've
>> noticed that
>> I must leave equivalences intact, even when transitioning to
>> VR_UNDEFINED:
>>
>> [from the attached patch]
>> + /* If THIS is varying we want to pick up equivalences from OTHER.
>> + Just special-case this here rather than trying to fixup
>> after the
>> + fact. */
>> + if (this->varying_p ())
>> + this->deep_copy (other);
>> + else if (this->undefined_p ())
>> + /* ?? Leave any equivalences already present in an undefined.
>> + This is technically not allowed, but we may get an in-flight
>> + value_range in an intermediate state. */
> Where/when does this happen?
The above snippet is not currently in mainline. It's in the patch I'm
proposing to clean up intersect. It's just that while cleaning up
intersect I noticed that if we keep to the value_range API, we end up
clobbering an equivalence to a VR_UNDEFINED that we depend up
further up
the call chain.
The reason it doesn't happen in mainline is because intersect_helper
bails early on an undefined, thus leaving the problematic equivalence
intact.
You can see it in mainline though, with the following testcase:
int f(int x)
{
if (x != 0 && x != 1)
return -2;
return !x;
}
Break in evrp_range_analyzer::record_ranges_from_stmt() and see that
the
call to extract_range_from_stmt() returns a VR of undefined *WITH*
equivalences:
vr_values->extract_range_from_stmt (stmt, &taken_edge,
&output, &vr);
This VR is later fed to update_value_range() and ultimately
intersect(),
which in mainline, leaves the equivalences alone, but IMO should
respect
that API and nuke them.
>>> So I think this is coming from extract_range_from_ssa_name:
>>>
void
vr_values::extract_range_from_ssa_nam
Re: undefined behavior in value_range::equiv_add()?
On 5/31/19 5:00 AM, Richard Biener wrote:
On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
On 5/29/19 10:20 AM, Aldy Hernandez wrote:
On 5/29/19 12:12 PM, Jeff Law wrote:
On 5/29/19 9:58 AM, Aldy Hernandez wrote:
On 5/29/19 9:24 AM, Richard Biener wrote:
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
wrote:
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences. However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect
and
we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
It looks like extract_range_from_stmt (by virtue of
vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
returns one of these intermediate ranges. It would seem to me that an
outward looking API method like vr_values::extract_range_from_stmt
shouldn't be returning inconsistent ranges. Or are there no guarantees
for value_ranges from within all of vr_values?
ISTM that if we have an implementation constraint that says a VR_VARYING
or VR_UNDEFINED range can't have equivalences, then we need to honor
that at the minimum for anything returned by an external API. Returning
an inconsistent state is bad. I'd even state that we should try damn
hard to avoid it in internal APIs as well.
Agreed * 2.
Perhaps I should give a little background. As part of your
value_range_base re-factoring last year, you mentioned that you didn't
split out intersect like you did union because of time or oversight. I
have code to implement intersect (attached), for which I've noticed that
I must leave equivalences intact, even when transitioning to
VR_UNDEFINED:
[from the attached patch]
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the
+ fact. */
+ if (this->varying_p ())
+this->deep_copy (other);
+ else if (this->undefined_p ())
+/* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
Where/when does this happen?
The above snippet is not currently in mainline. It's in the patch I'm
proposing to clean up intersect. It's just that while cleaning up
intersect I noticed that if we keep to the value_range API, we end up
clobbering an equivalence to a VR_UNDEFINED that we depend up further up
the call chain.
The reason it doesn't happen in mainline is because intersect_helper
bails early on an undefined, thus leaving the problematic equivalence
intact.
You can see it in mainline though, with the following testcase:
int f(int x)
{
if (x != 0 && x != 1)
return -2;
return !x;
}
Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
call to extract_range_from_stmt() returns a VR of undefined *WITH*
equivalences:
vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
This VR is later fed to update_value_range() and ultimately intersect(),
which in mainline, leaves the equivalences alone, but IMO should respect
that API and nuke them.
So I think this is coming from extract_range_from_ssa_name:
void
vr_values::extract_range_from_ssa_name (value_range *vr, tree var)
{
value_range *var_vr = get_value_range (var);
if (!var_vr->varying_p ())
vr->deep_copy (var_vr);
else
vr->set (var);
vr->equiv_add (var, get_value_range (var), &vrp_equiv_obstack);
}
Where we blindly add VAR to the equiv bitmap in the range.
AFAICT gcc-7 has equivalent code, it's just not inside the class.
I don't know what the fallout might be, but we could conditionalize the
call to add_equivalence to avoid the inconsistent state.
Well, the issue is that the equivalence _is_ there. So above we
know that we never end up with VARYING but the deep_copy
can bring over UNDEFINED to us. I guess we _could_ elide
the equiv adding then. OTOH the equivalence does look
useful for predicate simplification which IIRC doesn't treat
UNDEFINED != x in a useful wa
Re: undefined behavior in value_range::equiv_add()?
On 5/31/19 3:00 AM, Richard Biener wrote:
> On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
>>
>> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
>>> On 5/29/19 12:12 PM, Jeff Law wrote:
On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> On 5/29/19 9:24 AM, Richard Biener wrote:
>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
>> wrote:
>>>
>>> As per the API, and the original documentation to value_range,
>>> VR_UNDEFINED and VR_VARYING should never have equivalences. However,
>>> equiv_add is tacking on equivalences blindly, and there are various
>>> regressions that happen if I fix this oversight.
>>>
>>> void
>>> value_range::equiv_add (const_tree var,
>>> const value_range *var_vr,
>>> bitmap_obstack *obstack)
>>> {
>>> if (!m_equiv)
>>>m_equiv = BITMAP_ALLOC (obstack);
>>> unsigned ver = SSA_NAME_VERSION (var);
>>> bitmap_set_bit (m_equiv, ver);
>>> if (var_vr && var_vr->m_equiv)
>>>bitmap_ior_into (m_equiv, var_vr->m_equiv);
>>> }
>>>
>>> Is this a bug in the documentation / API, or is equiv_add incorrect
>>> and
>>> we should fix the fall-out elsewhere?
>>
>> I think this must have been crept in during the classification. If you
>> go back to say GCC 7 you shouldn't see value-ranges with
>> UNDEFINED/VARYING state in the lattice that have equivalences.
>>
>> It may not be easy to avoid with the new classy interface but we're
>> certainly not tacking on them "blindly". At least we're not supposed
>> to. As usual the intermediate state might be "broken" but
>> intermediateness is not sth the new class "likes".
>
> It looks like extract_range_from_stmt (by virtue of
> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
> returns one of these intermediate ranges. It would seem to me that an
> outward looking API method like vr_values::extract_range_from_stmt
> shouldn't be returning inconsistent ranges. Or are there no guarantees
> for value_ranges from within all of vr_values?
ISTM that if we have an implementation constraint that says a VR_VARYING
or VR_UNDEFINED range can't have equivalences, then we need to honor
that at the minimum for anything returned by an external API. Returning
an inconsistent state is bad. I'd even state that we should try damn
hard to avoid it in internal APIs as well.
>>>
>>> Agreed * 2.
>>>
>
> Perhaps I should give a little background. As part of your
> value_range_base re-factoring last year, you mentioned that you didn't
> split out intersect like you did union because of time or oversight. I
> have code to implement intersect (attached), for which I've noticed that
> I must leave equivalences intact, even when transitioning to
> VR_UNDEFINED:
>
> [from the attached patch]
> + /* If THIS is varying we want to pick up equivalences from OTHER.
> + Just special-case this here rather than trying to fixup after the
> + fact. */
> + if (this->varying_p ())
> +this->deep_copy (other);
> + else if (this->undefined_p ())
> +/* ?? Leave any equivalences already present in an undefined.
> + This is technically not allowed, but we may get an in-flight
> + value_range in an intermediate state. */
Where/when does this happen?
>>>
>>> The above snippet is not currently in mainline. It's in the patch I'm
>>> proposing to clean up intersect. It's just that while cleaning up
>>> intersect I noticed that if we keep to the value_range API, we end up
>>> clobbering an equivalence to a VR_UNDEFINED that we depend up further up
>>> the call chain.
>>>
>>> The reason it doesn't happen in mainline is because intersect_helper
>>> bails early on an undefined, thus leaving the problematic equivalence
>>> intact.
>>>
>>> You can see it in mainline though, with the following testcase:
>>>
>>> int f(int x)
>>> {
>>> if (x != 0 && x != 1)
>>> return -2;
>>>
>>> return !x;
>>> }
>>>
>>> Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
>>> call to extract_range_from_stmt() returns a VR of undefined *WITH*
>>> equivalences:
>>>
>>> vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
>>>
>>> This VR is later fed to update_value_range() and ultimately intersect(),
>>> which in mainline, leaves the equivalences alone, but IMO should respect
>>> that API and nuke them.
>> So I think this is coming from extract_range_from_ssa_name:
>>
>>> void
>>> vr_values::extract_range_from_ssa_name (value_range *vr, tree var)
>>> {
>>> value_range *var_vr = get_value_range (var);
>>>
>>> if (!var_vr->varying_p ())
>>> vr->deep_copy (var_vr);
>>> else
>>> vr->set (var);
>>>
>>> vr->equiv_add (var, get_value_range (var), &vrp_equiv_obstack);
>>>
Re: undefined behavior in value_range::equiv_add()?
On Fri, May 31, 2019 at 2:27 AM Jeff Law wrote:
>
> On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> > On 5/29/19 12:12 PM, Jeff Law wrote:
> >> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> >>> On 5/29/19 9:24 AM, Richard Biener wrote:
> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
> wrote:
> >
> > As per the API, and the original documentation to value_range,
> > VR_UNDEFINED and VR_VARYING should never have equivalences. However,
> > equiv_add is tacking on equivalences blindly, and there are various
> > regressions that happen if I fix this oversight.
> >
> > void
> > value_range::equiv_add (const_tree var,
> > const value_range *var_vr,
> > bitmap_obstack *obstack)
> > {
> > if (!m_equiv)
> >m_equiv = BITMAP_ALLOC (obstack);
> > unsigned ver = SSA_NAME_VERSION (var);
> > bitmap_set_bit (m_equiv, ver);
> > if (var_vr && var_vr->m_equiv)
> >bitmap_ior_into (m_equiv, var_vr->m_equiv);
> > }
> >
> > Is this a bug in the documentation / API, or is equiv_add incorrect
> > and
> > we should fix the fall-out elsewhere?
>
> I think this must have been crept in during the classification. If you
> go back to say GCC 7 you shouldn't see value-ranges with
> UNDEFINED/VARYING state in the lattice that have equivalences.
>
> It may not be easy to avoid with the new classy interface but we're
> certainly not tacking on them "blindly". At least we're not supposed
> to. As usual the intermediate state might be "broken" but
> intermediateness is not sth the new class "likes".
> >>>
> >>> It looks like extract_range_from_stmt (by virtue of
> >>> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
> >>> returns one of these intermediate ranges. It would seem to me that an
> >>> outward looking API method like vr_values::extract_range_from_stmt
> >>> shouldn't be returning inconsistent ranges. Or are there no guarantees
> >>> for value_ranges from within all of vr_values?
> >> ISTM that if we have an implementation constraint that says a VR_VARYING
> >> or VR_UNDEFINED range can't have equivalences, then we need to honor
> >> that at the minimum for anything returned by an external API. Returning
> >> an inconsistent state is bad. I'd even state that we should try damn
> >> hard to avoid it in internal APIs as well.
> >
> > Agreed * 2.
> >
> >>
> >>>
> >>> Perhaps I should give a little background. As part of your
> >>> value_range_base re-factoring last year, you mentioned that you didn't
> >>> split out intersect like you did union because of time or oversight. I
> >>> have code to implement intersect (attached), for which I've noticed that
> >>> I must leave equivalences intact, even when transitioning to
> >>> VR_UNDEFINED:
> >>>
> >>> [from the attached patch]
> >>> + /* If THIS is varying we want to pick up equivalences from OTHER.
> >>> + Just special-case this here rather than trying to fixup after the
> >>> + fact. */
> >>> + if (this->varying_p ())
> >>> +this->deep_copy (other);
> >>> + else if (this->undefined_p ())
> >>> +/* ?? Leave any equivalences already present in an undefined.
> >>> + This is technically not allowed, but we may get an in-flight
> >>> + value_range in an intermediate state. */
> >> Where/when does this happen?
> >
> > The above snippet is not currently in mainline. It's in the patch I'm
> > proposing to clean up intersect. It's just that while cleaning up
> > intersect I noticed that if we keep to the value_range API, we end up
> > clobbering an equivalence to a VR_UNDEFINED that we depend up further up
> > the call chain.
> >
> > The reason it doesn't happen in mainline is because intersect_helper
> > bails early on an undefined, thus leaving the problematic equivalence
> > intact.
> >
> > You can see it in mainline though, with the following testcase:
> >
> > int f(int x)
> > {
> > if (x != 0 && x != 1)
> > return -2;
> >
> > return !x;
> > }
> >
> > Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
> > call to extract_range_from_stmt() returns a VR of undefined *WITH*
> > equivalences:
> >
> > vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
> >
> > This VR is later fed to update_value_range() and ultimately intersect(),
> > which in mainline, leaves the equivalences alone, but IMO should respect
> > that API and nuke them.
> So I think this is coming from extract_range_from_ssa_name:
>
> > void
> > vr_values::extract_range_from_ssa_name (value_range *vr, tree var)
> > {
> > value_range *var_vr = get_value_range (var);
> >
> > if (!var_vr->varying_p ())
> > vr->deep_copy (var_vr);
> > else
> > vr->set (var);
> >
> > vr->equiv_add (var, get_value_range (var), &vrp_equiv_obstack);
> > }
>
> Where we blindly add VAR to the equiv bit
Re: undefined behavior in value_range::equiv_add()?
On 5/29/19 10:20 AM, Aldy Hernandez wrote:
> On 5/29/19 12:12 PM, Jeff Law wrote:
>> On 5/29/19 9:58 AM, Aldy Hernandez wrote:
>>> On 5/29/19 9:24 AM, Richard Biener wrote:
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez
wrote:
>
> As per the API, and the original documentation to value_range,
> VR_UNDEFINED and VR_VARYING should never have equivalences. However,
> equiv_add is tacking on equivalences blindly, and there are various
> regressions that happen if I fix this oversight.
>
> void
> value_range::equiv_add (const_tree var,
> const value_range *var_vr,
> bitmap_obstack *obstack)
> {
> if (!m_equiv)
> m_equiv = BITMAP_ALLOC (obstack);
> unsigned ver = SSA_NAME_VERSION (var);
> bitmap_set_bit (m_equiv, ver);
> if (var_vr && var_vr->m_equiv)
> bitmap_ior_into (m_equiv, var_vr->m_equiv);
> }
>
> Is this a bug in the documentation / API, or is equiv_add incorrect
> and
> we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
>>>
>>> It looks like extract_range_from_stmt (by virtue of
>>> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
>>> returns one of these intermediate ranges. It would seem to me that an
>>> outward looking API method like vr_values::extract_range_from_stmt
>>> shouldn't be returning inconsistent ranges. Or are there no guarantees
>>> for value_ranges from within all of vr_values?
>> ISTM that if we have an implementation constraint that says a VR_VARYING
>> or VR_UNDEFINED range can't have equivalences, then we need to honor
>> that at the minimum for anything returned by an external API. Returning
>> an inconsistent state is bad. I'd even state that we should try damn
>> hard to avoid it in internal APIs as well.
>
> Agreed * 2.
>
>>
>>>
>>> Perhaps I should give a little background. As part of your
>>> value_range_base re-factoring last year, you mentioned that you didn't
>>> split out intersect like you did union because of time or oversight. I
>>> have code to implement intersect (attached), for which I've noticed that
>>> I must leave equivalences intact, even when transitioning to
>>> VR_UNDEFINED:
>>>
>>> [from the attached patch]
>>> + /* If THIS is varying we want to pick up equivalences from OTHER.
>>> + Just special-case this here rather than trying to fixup after the
>>> + fact. */
>>> + if (this->varying_p ())
>>> + this->deep_copy (other);
>>> + else if (this->undefined_p ())
>>> + /* ?? Leave any equivalences already present in an undefined.
>>> + This is technically not allowed, but we may get an in-flight
>>> + value_range in an intermediate state. */
>> Where/when does this happen?
>
> The above snippet is not currently in mainline. It's in the patch I'm
> proposing to clean up intersect. It's just that while cleaning up
> intersect I noticed that if we keep to the value_range API, we end up
> clobbering an equivalence to a VR_UNDEFINED that we depend up further up
> the call chain.
>
> The reason it doesn't happen in mainline is because intersect_helper
> bails early on an undefined, thus leaving the problematic equivalence
> intact.
>
> You can see it in mainline though, with the following testcase:
>
> int f(int x)
> {
> if (x != 0 && x != 1)
> return -2;
>
> return !x;
> }
>
> Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
> call to extract_range_from_stmt() returns a VR of undefined *WITH*
> equivalences:
>
> vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
>
> This VR is later fed to update_value_range() and ultimately intersect(),
> which in mainline, leaves the equivalences alone, but IMO should respect
> that API and nuke them.
So I think this is coming from extract_range_from_ssa_name:
> void
> vr_values::extract_range_from_ssa_name (value_range *vr, tree var)
> {
> value_range *var_vr = get_value_range (var);
>
> if (!var_vr->varying_p ())
> vr->deep_copy (var_vr);
> else
> vr->set (var);
>
> vr->equiv_add (var, get_value_range (var), &vrp_equiv_obstack);
> }
Where we blindly add VAR to the equiv bitmap in the range.
AFAICT gcc-7 has equivalent code, it's just not inside the class.
I don't know what the fallout might be, but we could conditionalize the
call to add_equivalence to avoid the inconsistent state.
Jeff
Re: undefined behavior in value_range::equiv_add()?
On 5/29/19 12:12 PM, Jeff Law wrote:
On 5/29/19 9:58 AM, Aldy Hernandez wrote:
On 5/29/19 9:24 AM, Richard Biener wrote:
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote:
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences. However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect and
we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
It looks like extract_range_from_stmt (by virtue of
vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
returns one of these intermediate ranges. It would seem to me that an
outward looking API method like vr_values::extract_range_from_stmt
shouldn't be returning inconsistent ranges. Or are there no guarantees
for value_ranges from within all of vr_values?
ISTM that if we have an implementation constraint that says a VR_VARYING
or VR_UNDEFINED range can't have equivalences, then we need to honor
that at the minimum for anything returned by an external API. Returning
an inconsistent state is bad. I'd even state that we should try damn
hard to avoid it in internal APIs as well.
Agreed * 2.
Perhaps I should give a little background. As part of your
value_range_base re-factoring last year, you mentioned that you didn't
split out intersect like you did union because of time or oversight. I
have code to implement intersect (attached), for which I've noticed that
I must leave equivalences intact, even when transitioning to VR_UNDEFINED:
[from the attached patch]
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the
+ fact. */
+ if (this->varying_p ())
+ this->deep_copy (other);
+ else if (this->undefined_p ())
+ /* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
Where/when does this happen?
The above snippet is not currently in mainline. It's in the patch I'm
proposing to clean up intersect. It's just that while cleaning up
intersect I noticed that if we keep to the value_range API, we end up
clobbering an equivalence to a VR_UNDEFINED that we depend up further up
the call chain.
The reason it doesn't happen in mainline is because intersect_helper
bails early on an undefined, thus leaving the problematic equivalence
intact.
You can see it in mainline though, with the following testcase:
int f(int x)
{
if (x != 0 && x != 1)
return -2;
return !x;
}
Break in evrp_range_analyzer::record_ranges_from_stmt() and see that the
call to extract_range_from_stmt() returns a VR of undefined *WITH*
equivalences:
vr_values->extract_range_from_stmt (stmt, &taken_edge, &output, &vr);
This VR is later fed to update_value_range() and ultimately intersect(),
which in mainline, leaves the equivalences alone, but IMO should respect
that API and nuke them.
For my proposed overhaul of intersect, I have to special-case the
undefined to make sure we don't clobber it's inconsistent use of
equivalences.
+ ;
What is happening is that in evrp's record_ranges_from_stmt, we call
extract_range_from_stmt which returns an inconsistent VR_UNDEFINED with
an equivalence, which is then fed to update_value_range() and finally
value_range::intersect. The VR_UNDEFINED equivalence must not be
removed in the intersect, because update_value_range() will get confused
as to whether this is a new VR or not (because VR_UNDEFINED with no
equivalences is not the same as VR_UNDEFINED with equivalences-- see
"is_new" in update_value_range).
Ugh. I hate some of the gyrations we have to do for update_value_range.
Regardless I tend to think the problem is in the inconsistent state we
get back from extract_range_from_stmt.
Agreed.
Aldy
Re: undefined behavior in value_range::equiv_add()?
On 5/29/19 9:58 AM, Aldy Hernandez wrote:
> On 5/29/19 9:24 AM, Richard Biener wrote:
>> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote:
>>>
>>> As per the API, and the original documentation to value_range,
>>> VR_UNDEFINED and VR_VARYING should never have equivalences. However,
>>> equiv_add is tacking on equivalences blindly, and there are various
>>> regressions that happen if I fix this oversight.
>>>
>>> void
>>> value_range::equiv_add (const_tree var,
>>> const value_range *var_vr,
>>> bitmap_obstack *obstack)
>>> {
>>> if (!m_equiv)
>>> m_equiv = BITMAP_ALLOC (obstack);
>>> unsigned ver = SSA_NAME_VERSION (var);
>>> bitmap_set_bit (m_equiv, ver);
>>> if (var_vr && var_vr->m_equiv)
>>> bitmap_ior_into (m_equiv, var_vr->m_equiv);
>>> }
>>>
>>> Is this a bug in the documentation / API, or is equiv_add incorrect and
>>> we should fix the fall-out elsewhere?
>>
>> I think this must have been crept in during the classification. If you
>> go back to say GCC 7 you shouldn't see value-ranges with
>> UNDEFINED/VARYING state in the lattice that have equivalences.
>>
>> It may not be easy to avoid with the new classy interface but we're
>> certainly not tacking on them "blindly". At least we're not supposed
>> to. As usual the intermediate state might be "broken" but
>> intermediateness is not sth the new class "likes".
>
> It looks like extract_range_from_stmt (by virtue of
> vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
> returns one of these intermediate ranges. It would seem to me that an
> outward looking API method like vr_values::extract_range_from_stmt
> shouldn't be returning inconsistent ranges. Or are there no guarantees
> for value_ranges from within all of vr_values?
ISTM that if we have an implementation constraint that says a VR_VARYING
or VR_UNDEFINED range can't have equivalences, then we need to honor
that at the minimum for anything returned by an external API. Returning
an inconsistent state is bad. I'd even state that we should try damn
hard to avoid it in internal APIs as well.
>
> Perhaps I should give a little background. As part of your
> value_range_base re-factoring last year, you mentioned that you didn't
> split out intersect like you did union because of time or oversight. I
> have code to implement intersect (attached), for which I've noticed that
> I must leave equivalences intact, even when transitioning to VR_UNDEFINED:
>
> [from the attached patch]
> + /* If THIS is varying we want to pick up equivalences from OTHER.
> + Just special-case this here rather than trying to fixup after the
> + fact. */
> + if (this->varying_p ())
> + this->deep_copy (other);
> + else if (this->undefined_p ())
> + /* ?? Leave any equivalences already present in an undefined.
> + This is technically not allowed, but we may get an in-flight
> + value_range in an intermediate state. */
Where/when does this happen?
> + ;
>
> What is happening is that in evrp's record_ranges_from_stmt, we call
> extract_range_from_stmt which returns an inconsistent VR_UNDEFINED with
> an equivalence, which is then fed to update_value_range() and finally
> value_range::intersect. The VR_UNDEFINED equivalence must not be
> removed in the intersect, because update_value_range() will get confused
> as to whether this is a new VR or not (because VR_UNDEFINED with no
> equivalences is not the same as VR_UNDEFINED with equivalences-- see
> "is_new" in update_value_range).
Ugh. I hate some of the gyrations we have to do for update_value_range.
Regardless I tend to think the problem is in the inconsistent state we
get back from extract_range_from_stmt.
Jeff
Re: undefined behavior in value_range::equiv_add()?
On 5/29/19 7:24 AM, Richard Biener wrote:
> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote:
>>
>> As per the API, and the original documentation to value_range,
>> VR_UNDEFINED and VR_VARYING should never have equivalences. However,
>> equiv_add is tacking on equivalences blindly, and there are various
>> regressions that happen if I fix this oversight.
>>
>> void
>> value_range::equiv_add (const_tree var,
>> const value_range *var_vr,
>> bitmap_obstack *obstack)
>> {
>>if (!m_equiv)
>> m_equiv = BITMAP_ALLOC (obstack);
>>unsigned ver = SSA_NAME_VERSION (var);
>>bitmap_set_bit (m_equiv, ver);
>>if (var_vr && var_vr->m_equiv)
>> bitmap_ior_into (m_equiv, var_vr->m_equiv);
>> }
>>
>> Is this a bug in the documentation / API, or is equiv_add incorrect and
>> we should fix the fall-out elsewhere?
>
> I think this must have been crept in during the classification. If you
> go back to say GCC 7 you shouldn't see value-ranges with
> UNDEFINED/VARYING state in the lattice that have equivalences.
>
> It may not be easy to avoid with the new classy interface but we're
> certainly not tacking on them "blindly". At least we're not supposed
> to. As usual the intermediate state might be "broken" but
> intermediateness is not sth the new class "likes".
I don't remember changing anything (behavior-wise) in this space. If I
did it certainly wasn't intentional.
Given the code in gcc-7 looks like this:
> static void
> add_equivalence (bitmap *equiv, const_tree var)
> {
> unsigned ver = SSA_NAME_VERSION (var);
> value_range *vr = get_value_range (var);
>
> if (*equiv == NULL)
> *equiv = BITMAP_ALLOC (&vrp_equiv_obstack);
> bitmap_set_bit (*equiv, ver);
> if (vr && vr->equiv)
> bitmap_ior_into (*equiv, vr->equiv);
> }
I suspect we need to look up the call chain.
Jeff
Re: undefined behavior in value_range::equiv_add()?
On 5/29/19 9:24 AM, Richard Biener wrote:
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote:
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences. However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect and
we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
It looks like extract_range_from_stmt (by virtue of
vrp_visit_assignment_or_call and then extract_range_from_ssa_name)
returns one of these intermediate ranges. It would seem to me that an
outward looking API method like vr_values::extract_range_from_stmt
shouldn't be returning inconsistent ranges. Or are there no guarantees
for value_ranges from within all of vr_values?
Perhaps I should give a little background. As part of your
value_range_base re-factoring last year, you mentioned that you didn't
split out intersect like you did union because of time or oversight. I
have code to implement intersect (attached), for which I've noticed that
I must leave equivalences intact, even when transitioning to VR_UNDEFINED:
[from the attached patch]
+ /* If THIS is varying we want to pick up equivalences from OTHER.
+ Just special-case this here rather than trying to fixup after the
+ fact. */
+ if (this->varying_p ())
+this->deep_copy (other);
+ else if (this->undefined_p ())
+/* ?? Leave any equivalences already present in an undefined.
+ This is technically not allowed, but we may get an in-flight
+ value_range in an intermediate state. */
+;
What is happening is that in evrp's record_ranges_from_stmt, we call
extract_range_from_stmt which returns an inconsistent VR_UNDEFINED with
an equivalence, which is then fed to update_value_range() and finally
value_range::intersect. The VR_UNDEFINED equivalence must not be
removed in the intersect, because update_value_range() will get confused
as to whether this is a new VR or not (because VR_UNDEFINED with no
equivalences is not the same as VR_UNDEFINED with equivalences-- see
"is_new" in update_value_range).
I'd rather not special case VR_UNDEFINED in the intersect code as above,
but if you think extract_range_from_stmt() can return an "intermediate"
range, and thus intersect must handle them too, then I suppose we could
leave this in.
What do you suggest?
Oh yeah, is the attached patch OK for trunk? I can post in a separate
thread if you prefer, but thought it relevant here :).
Aldy
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 9f194540327..9494520ba33 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,19 @@
+2019-05-29 Aldy Hernandez
+
+ * tree-vrp.h (value_range_base::intersect): New.
+ (value_range::intersect_helper): Move from here...
+ (value_range_base::intersect_helper): ...to here.
+ * tree-vrp.c (value_range::intersect_helper): Rename to...
+ (value_range_base::intersect_helper): ...this, and rewrite to
+ return a value instead of modifying THIS in place.
+ Also, move equivalence handling...
+ (value_range::intersect): ...here, while calling intersect_helper.
+ * gimple-fold.c (size_must_be_zero_p): Use value_range_base when
+ calling intersect.
+ * gimple-ssa-evrp-analyze.c (ecord_ranges_from_incoming_edge):
+ Same.
+ * vr-values.c (vrp_evaluate_conditional_warnv_with_ops): Same.
+
2019-05-29 Aldy Hernandez
* tree-vrp.h (value_range_base::non_zero_p): New.
* tree-vrp.c (range_is_null): Remove.
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index b3e931744f8..8b8331eb555 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -684,10 +684,10 @@ size_must_be_zero_p (tree size)
/* Compute the value of SSIZE_MAX, the largest positive value that
can be stored in ssize_t, the signed counterpart of size_t. */
wide_int ssize_max = wi::lshift (wi::one (prec), prec - 1) - 1;
- value_range valid_range (VR_RANGE,
- build_int_cst (type, 0),
- wide_int_to_tree (type, ssize_max));
- value_range vr;
+ value_range_base valid_range (VR_RANGE,
+build_int_cst (type, 0),
+wide_int_to_tree (t
Re: undefined behavior in value_range::equiv_add()?
On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote:
>
> As per the API, and the original documentation to value_range,
> VR_UNDEFINED and VR_VARYING should never have equivalences. However,
> equiv_add is tacking on equivalences blindly, and there are various
> regressions that happen if I fix this oversight.
>
> void
> value_range::equiv_add (const_tree var,
> const value_range *var_vr,
> bitmap_obstack *obstack)
> {
>if (!m_equiv)
> m_equiv = BITMAP_ALLOC (obstack);
>unsigned ver = SSA_NAME_VERSION (var);
>bitmap_set_bit (m_equiv, ver);
>if (var_vr && var_vr->m_equiv)
> bitmap_ior_into (m_equiv, var_vr->m_equiv);
> }
>
> Is this a bug in the documentation / API, or is equiv_add incorrect and
> we should fix the fall-out elsewhere?
I think this must have been crept in during the classification. If you
go back to say GCC 7 you shouldn't see value-ranges with
UNDEFINED/VARYING state in the lattice that have equivalences.
It may not be easy to avoid with the new classy interface but we're
certainly not tacking on them "blindly". At least we're not supposed
to. As usual the intermediate state might be "broken" but
intermediateness is not sth the new class "likes".
Richard.
>
> Thanks.
> Aldy
undefined behavior in value_range::equiv_add()?
As per the API, and the original documentation to value_range,
VR_UNDEFINED and VR_VARYING should never have equivalences. However,
equiv_add is tacking on equivalences blindly, and there are various
regressions that happen if I fix this oversight.
void
value_range::equiv_add (const_tree var,
const value_range *var_vr,
bitmap_obstack *obstack)
{
if (!m_equiv)
m_equiv = BITMAP_ALLOC (obstack);
unsigned ver = SSA_NAME_VERSION (var);
bitmap_set_bit (m_equiv, ver);
if (var_vr && var_vr->m_equiv)
bitmap_ior_into (m_equiv, var_vr->m_equiv);
}
Is this a bug in the documentation / API, or is equiv_add incorrect and
we should fix the fall-out elsewhere?
Thanks.
Aldy
