This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: types for VR_VARYING
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?!
Sorry, couldn't resist to chime in when seeing the NOP_EXPR build.
Last time. Promised. I'm resisting to comment on the supports_type_p
abdomination.
Richard.
> Aldy