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. 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. > 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. Jeff