This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: types for VR_VARYING


On 8/15/19 10:06 AM, Aldy Hernandez wrote:
> 
> 
> On 8/15/19 7:23 AM, Richard Biener wrote:
>> On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> wrote:
>>>
>>> On 8/14/19 1:37 PM, Jeff Law wrote:
>>>> On 8/13/19 6:39 PM, Aldy Hernandez wrote:
>>>>>
>>>>>
>>>>> On 8/12/19 7:46 PM, Jeff Law wrote:
>>>>>> On 8/12/19 12:43 PM, Aldy Hernandez wrote:
>>>>>>> This is a fresh re-post of:
>>>>>>>
>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00006.html
>>>>>>>
>>>>>>> Andrew gave me some feedback a week ago, and I obviously don't
>>>>>>> remember
>>>>>>> what it was because I was about to leave on PTO.  However, I do
>>>>>>> remember
>>>>>>> I addressed his concerns before getting drunk on rum in tropical
>>>>>>> islands.
>>>>>>>
>>>>>> FWIW found a great coffee infused rum while in Kauai last week. 
>>>>>> I'm not
>>>>>> a coffee fan, but it was wonderful.  The one bottle we brought back
>>>>>> isn't going to last until Cauldron and I don't think I can get a
>>>>>> special
>>>>>> order filled before I leave :(
>>>>>
>>>>> You must bring some to Cauldron before we believe you. :)
>>>> That's the problem.  The nearest place I can get it is in Vegas and
>>>> there's no distributor in Montreal.   I can special order it in our
>>>> state run stores, but it won't be here in time.
>>>>
>>>> Of course, I don't mind if you don't believe me.  More for me in that
>>>> case...
>>>>
>>>>
>>>>>> Is the supports_type_p stuff there to placate the calls from
>>>>>> ipa-cp?  I
>>>>>> can live with it in the short term, but it really feels like there
>>>>>> should be something in the ipa-cp client that avoids this silliness.
>>>>>
>>>>> I am not happy with this either, but there are various places where
>>>>> statements that are !stmt_interesting_for_vrp() are still setting a
>>>>> range of VARYING, which is then being ignored at a later time.
>>>>>
>>>>> For example, vrp_initialize:
>>>>>
>>>>>         if (!stmt_interesting_for_vrp (phi))
>>>>>           {
>>>>>             tree lhs = PHI_RESULT (phi);
>>>>>             set_def_to_varying (lhs);
>>>>>             prop_set_simulate_again (phi, false);
>>>>>           }
>>>>>
>>>>> Also in evrp_range_analyzer::record_ranges_from_stmt(), where we if
>>>>> the
>>>>> statement is interesting for VRP but extract_range_from_stmt() does
>>>>> not
>>>>> produce a useful range, we also set a varying for a range we will
>>>>> never
>>>>> use.  Similarly for a statement that is not interesting in this hunk.
>>>> Ugh.  One could perhaps argue that setting any kind of range in these
>>>> circumstances is silly.   But I suspect it's necessary due to the
>>>> optimistic handling of VR_UNDEFINED in value_range_base::union_helper.
>>>> It's all coming back to me now...
>>>>
>>>>
>>>>>
>>>>> Then there is vrp_prop::visit_stmt() where we also set VARYING for
>>>>> types
>>>>> that VRP will never handle:
>>>>>
>>>>>         case IFN_ADD_OVERFLOW:
>>>>>         case IFN_SUB_OVERFLOW:
>>>>>         case IFN_MUL_OVERFLOW:
>>>>>         case IFN_ATOMIC_COMPARE_EXCHANGE:
>>>>>       /* These internal calls return _Complex integer type,
>>>>>          which VRP does not track, but the immediate uses
>>>>>          thereof might be interesting.  */
>>>>>       if (lhs && TREE_CODE (lhs) == SSA_NAME)
>>>>>         {
>>>>>           imm_use_iterator iter;
>>>>>           use_operand_p use_p;
>>>>>           enum ssa_prop_result res = SSA_PROP_VARYING;
>>>>>
>>>>>           set_def_to_varying (lhs);
>>>>>
>>>>> I've adjusted the patch so that set_def_to_varying will set the
>>>>> range to
>>>>> VR_UNDEFINED if !supports_type_p.  This is a fail safe, as we can't
>>>>> really do anything with a nonsensical range.  I just don't want to
>>>>> leave
>>>>> the range in an indeterminate state.
>>>>>
>>>> I think VR_UNDEFINED is unsafe due to value_range_base::union_helper.
>>>> And that's a more general than this patch.  VR_UNDEFINED is _not_ a
>>>> safe
>>>> range to set something to if we can't handle it.  We have to use
>>>> VR_VARYING.
>>>>
>>>> Why?  See the beginning of value_range_base::union_helper:
>>>>
>>>>      /* VR0 has the resulting range if VR1 is undefined or VR0 is
>>>> varying.  */
>>>>      if (vr1->undefined_p ()
>>>>          || vr0->varying_p ())
>>>>        return *vr0;
>>>>
>>>>      /* VR1 has the resulting range if VR0 is undefined or VR1 is
>>>> varying.  */
>>>>      if (vr0->undefined_p ()
>>>>          || vr1->varying_p ())
>>>>        return *vr1;
>>>> This can get called for something like
>>>>
>>>>     a = <cond> ? name1 : name2;
>>>>
>>>> If name1 was set to VR_UNDEFINED thinking that VR_UNDEFINED was a safe
>>>> value for something we can't handle, then we'll incorrectly return the
>>>> range for name2.
>>>
>>> I think that if name1 was !supports_type_p, we will have never called
>>> union/intersect.  We will have bailed at some point earlier.  However, I
>>> do see your point about being consistent.
>>>
>>>>
>>>> VR_UNDEFINED can only be used for the ranges of objects we haven't
>>>> processed.  If we can't produce a range for an object because the
>>>> statement is something we don't handle or just doesn't produce anythign
>>>> useful, then the right result is VR_VARYING.
>>>>
>>>> This may be worth commenting at the definition site for VR_*.
>>>>
>>>>>
>>>>> I also noticed that Andrew's patch was setting num_vr_values to
>>>>> num_ssa_names + num_ssa_names / 10.  I think he meant num_vr_values +
>>>>> num_vr_values / 10.  Please verify the current incantation makes
>>>>> sense.
>>>> Going to assume this will be adjusted per the other messages in this
>>>> thread.
>>>
>>> Done.
>>>
>>>>
>>>>
>>>>> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
>>>>> index 39ea22f0554..663dd6e2398 100644
>>>>> --- a/gcc/tree-ssa-threadedge.c
>>>>> +++ b/gcc/tree-ssa-threadedge.c
>>>>> @@ -182,8 +182,10 @@ record_temporary_equivalences_from_phis (edge e,
>>>>>           new_vr->deep_copy (vr_values->get_value_range (src));
>>>>>         else if (TREE_CODE (src) == INTEGER_CST)
>>>>>           new_vr->set (src);
>>>>> +      else if (value_range_base::supports_type_p (TREE_TYPE (src)))
>>>>> +        new_vr->set_varying (TREE_TYPE (src));
>>>>>         else
>>>>> -        new_vr->set_varying ();
>>>>> +        new_vr->set_undefined ();
>>>> So I think this can cause problems.  VR_VARYING seems like the right
>>>> state here.
>>>>
>>>>
>>>>> +
>>>>> +  if (!value_range_base::supports_type_p (TREE_TYPE (var)))
>>>>> +    {
>>>>> +      vr->set_undefined ();
>>>>> +      return vr;
>>>> Probably better as VR_VARYING here too.
>>>>
>>>>> +    {
>>>>> +      /* If we have an unsupported type (structs, void, etc), there
>>>>> +         is nothing we'll be able to do with this entry.
>>>>> +         Initialize it to UNDEFINED as a sanity measure, just in
>>>>> +         case.  */
>>>>> +      vr->set_undefined ();
>>>> Here too.
>>>
>>> Hmmm, the problem with setting VR_VARYING for unsupported types is that
>>> we have no min/max to use.  Even though min/max will not be used in any
>>> calculation, it's nice to have it set so type() will work consistently.
>>> May I suggest this generic approach while we disassociate the lattice
>>> and ranges from value_range_base (or remove vrp altogether :))?
>>>
>>> void
>>> value_range_base::set_varying (tree type)
>>> {
>>>     m_kind = VR_VARYING;
>>>     if (supports_type_p (type))
>>>       {
>>>         m_min = vrp_val_min (type, true);
>>>         m_max = vrp_val_max (type, true);
>>>       }
>>>     else
>>>       {
>>>         /* We can't do anything range-wise with these types.  Build
>>>           something for which type() will work as a temporary measure
>>>           until lattices and value_range_base are disassociated.  */
>>>         m_min = m_max = build1 (NOP_EXPR, type, void_type_node);
>>>       }
>>> }
>>>
>>> This way, no changes happen throughout the code, varying remains
>>> varying, type() works everywhere, and we don't have to dig into all
>>> value_range users to skip unsupported types.
>>>
>>> This should work, as no one is going to call min() / max() on an
>>> unsupported type, since they're just being used for the lattice.
>>
>> Then use error_mark_node or NULL_TREE?!
> 
> I tested with error_mark_node.  It seems no one is calling type() on
> these unsupported types, so perhaps error_mark_node is fine, and signals
> that we don't support these types if anyone tries to do anything funny
> with them.  I've adjusted the patch.
I like error_mark_node.  It makes it pretty clear that something has
gone horribly wrong as opposed to something perhaps being uninitialized.

jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]