On 5/31/19 3:00 AM, Richard Biener wrote:
> On Fri, May 31, 2019 at 2:27 AM Jeff Law <l...@redhat.com> 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 <al...@redhat.com>
>>>>>> 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 way. Which would mean allowing
> equivalences on UNDEFINED. That somewhat makes sense
> since UNDEFINED is bottom-most in the lattice and one up
> (CONSTANT) we have equivalences while just on topmost
> (VARYING) we do not.
>
> That said, I never liked equivalences - they are an odd way
> to represent ranges intersected from multiple ranges thus
> a way out of our too simplistic range representation.
>
> So lets fix this in a way avoiding testsuite fallout. But please
> not with special hacks in consumers. Does guariding the
> above equiv_add with !vr->undefined_p () fix things?
I think we're in total agreement -- fix the inconsistencies, not the
consumers.
Jeff
