types for VR_VARYING

[Aldy Hernandez]

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. :)

> 
> Their more traditional rums were good as well.  Koloa Rum.  See if you
> can get it locally :-)
> 
> 
>>
>> This patch adds MIN/MAX values for VR_VARYING.  As was discussed
>> earlier, we are only changing VR_VARYING, not VR_UNDEFINED as was the
>> original plan.
>>
>> As I mentioned earlier, I am tired of re-doing ChangeLogs, so I'll whip
>> up the changelog when the review starts.
> ACK.

ChangeLog entries included in attached patch.

> 
> 
> 
>> @@ -150,12 +166,11 @@ vr_values::set_defs_to_varying (gimple *stmt)
>>     ssa_op_iter i;
>>     tree def;
>>     FOR_EACH_SSA_TREE_OPERAND (def, stmt, i, SSA_OP_DEF)
>> -    {
>> -      value_range *vr = get_value_range (def);
>> -      /* Avoid writing to vr_const_varying get_value_range may return.  */
>> -      if (!vr->varying_p ())
>> -	vr->set_varying ();
>> -    }
>> +    if (value_range_base::supports_type_p (TREE_TYPE (def)))
>> +      {
>> +	value_range *vr = get_value_range (def);
>> +	vr->set_varying (TREE_TYPE (def));
>> +      }
>>   }
> 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.

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.

> 
> 
>>   
>>   /* Update the value range and equivalence set for variable VAR to
> 
>> @@ -1920,7 +1935,7 @@ vr_values::dump_all_value_ranges (FILE *file)
>>   vr_values::vr_values () : vrp_value_range_pool ("Tree VRP value ranges")
>>   {
>>     values_propagated = false;
>> -  num_vr_values = num_ssa_names;
>> +  num_vr_values = num_ssa_names + num_ssa_names / 10;
>>     vr_value = XCNEWVEC (value_range *, num_vr_values);
>>     vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names);
>>     bitmap_obstack_initialize (&vrp_equiv_obstack);
> My recollection was someone (Richi) proposed 2X for the initial
> allocation which should ensure that in all but the most pathological
> cases that we never trigger a reallocation.  In terms of "wasted" space,
> it shouldn't matter in practice.
> 
> So if you could check the old thread and verify that Richi recommended
> the 2X initial allocation and if so, make that minor update.

Yes, it was 2X.

I noticed that Richi made some changes to the lattice handling for 
VARYING while the discussion was on-going.  I missed these, and had 
failed to adapt the patch for it.  I would appreciate a final review of 
the attached patch, especially the vr-values.c changes, which I have 
modified to play nice with current trunk.

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.

Tested on x86-64 Linux.

OK for trunk?

Aldy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: curr.patch
Type: text/x-patch
Size: 29373 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20190814/29ec0145/attachment.bin>