Re: [ping][PATCH][1 of 2] Add value range info to SSA_NAME for zero sign extension elimination in RTL

[Richard Biener <rguenther at suse dot de>]

On 9/3/13 2:15 PM, Kugan wrote:
> Thanks Richard for reviewing.
> 
> On 02/09/13 22:15, Richard Biener wrote:
>> On Wed, Jul 3, 2013 at 2:25 PM, Kugan
>> <kugan.vivekanandarajah@linaro.org> wrote:
>>> On 17/06/13 18:33, Richard Biener wrote:
>>>>
>>>> On Mon, 17 Jun 2013, Kugan wrote:
>>>> +/* Extract the value range of assigned exprassion for GIMPLE_ASSIGN
>>>> stmt.
>>>> +   If the extracted value range is valid, return true else return
>>>> +   false.  */
>>>> +static bool
>>>> +extract_exp_value_range (gimple stmt, value_range_t *vr)
>>>> +{
>>>> +  gcc_assert (is_gimple_assign (stmt));
>>>> +  tree rhs1 = gimple_assign_rhs1 (stmt);
>>>> +  tree lhs = gimple_assign_lhs (stmt);
>>>> +  enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
>>>> ...
>>>> @@ -8960,6 +9016,23 @@ simplify_stmt_using_ranges (gimple_stmt_iterator
>>>> *gsi)
>>>>        {
>>>>          enum tree_code rhs_code = gimple_assign_rhs_code (stmt);
>>>>          tree rhs1 = gimple_assign_rhs1 (stmt);
>>>> +      tree lhs = gimple_assign_lhs (stmt);
>>>> +
>>>> +      /* Set value range information for ssa.  */
>>>> +      if (!POINTER_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))
>>>> +          && (TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
>>>> +          && INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))
>>>> +          && !SSA_NAME_RANGE_INFO (lhs))
>>>> +        {
>>>> +          value_range_t vr = VR_INITIALIZER;
>>>> ...
>>>> +          if (extract_exp_value_range (stmt, &vr))
>>>> +            tree_ssa_set_value_range (lhs,
>>>> +                                      tree_to_double_int (vr.min),
>>>> +                                      tree_to_double_int (vr.max),
>>>> +                                      vr.type == VR_RANGE);
>>>> +        }
>>>>
>>>> This looks overly complicated to me.  In vrp_finalize you can simply do
>>>>
>>>>     for (i = 0; i < num_vr_values; i++)
>>>>       if (vr_value[i])
>>>>         {
>>>>           tree name = ssa_name (i);
>>>>           if (POINTER_TYPE_P (name))
>>>>             continue;
>>>>           if (vr_value[i].type == VR_RANGE
>>>>               || vr_value[i].type == VR_ANTI_RANGE)
>>>>             tree_ssa_set_value_range (name, tree_to_double_int
>>>> (vr_value[i].min), tree_to_double_int (vr_value[i].max),
>>>> vr_value[i].type
>>>> == VR_RANGE);
>>>>         }
>>>>
>>>
>>> Thanks Richard for taking time to review it.
>>>
>>> I was doing something like what you are suggesting earlier but
>>> noticed some
>>> problems and that’s the reason why I changed.
>>>
>>> For example, for the following testcase from the test suite,
>>>
>>> unsigned long l = (unsigned long)-2;
>>> unsigned short s;
>>>
>>> int main () {
>>>    long t = l + 1;
>>>    s = l;
>>>    if (s != (unsigned short) -2)
>>>      abort ();
>>>    exit (0);
>>> }
>>>
>>> with the following gimple stmts
>>>
>>> main ()
>>> {
>>>    short unsigned int s.1;
>>>    long unsigned int l.0;
>>>
>>> ;;   basic block 2, loop depth 0
>>> ;;    pred:       ENTRY
>>>    l.0_2 = l;
>>>    s.1_3 = (short unsigned int) l.0_2;
>>>    s = s.1_3;
>>>    if (s.1_3 != 65534)
>>>      goto <bb 3>;
>>>    else
>>>      goto <bb 4>;
>>> ;;    succ:       3
>>> ;;                4
>>>
>>> ;;   basic block 3, loop depth 0
>>> ;;    pred:       2
>>>    abort ();
>>> ;;    succ:
>>>
>>> ;;   basic block 4, loop depth 0
>>> ;;    pred:       2
>>>    exit (0);
>>> ;;    succ:
>>>
>>> }
>>>
>>>
>>>
>>> has the following value range.
>>>
>>> l.0_2: VARYING
>>> s.1_3: [0, +INF]
>>>
>>>
>>>  From zero/sign extension point of view, the variable s.1_3 is
>>> expected to
>>> have a value that will overflow (or varying) as this is what is
>>> assigned to
>>> a smaller variable. extract_range_from_assignment initially
>>> calculates the
>>> value range as VARYING but later changed to [0, +INF] by
>>> extract_range_basic. What I need here is the value that will be assigned
>>> from the rhs expression and not the value that we will have with proper
>>> assignment.
>>
>> I don't understand this.  The relevant statement is
>>
>>    s.1_3 = (short unsigned int) l.0_2;
>>
>> right?  You have value-ranges for both s.1_3 and l.0_2 as above.  And
>> you clearly cannot optimize the truncation away (and if you could,
>> you wond't need value-range information for that fact).
>>
> This is true. But just by looking at the value range of s.1.3 we will
> only see [0 +INF], as we are transferring directly from the lattice to
> lhs its value range.
> 
> [0, +INF] here tells us  vrp_val_is_max and it is not
> is_positive_overflow_infinity (or varying). Thats why we need to get the
> value range of RHS expression which will tell us the actual range. We
> can then use this range and see of we can fit it to lhs type without
> truncation.

Well, my point is you want to look at the l.0_2 value-range for this.
Storing the l.0_2 value-range for s.1_3 is wrong.

>>> I understand that the above code of mine needs to be changed but not
>>> convinced about the best way to do that.
>>>
>>> I can possibly re-factor extract_range_from_assignment to give me this
>>> information with an additional argument. Could you kindly let me know
>>> your
>>> preference.
>>>
>>>
>>>
>>>>
>>>> /* SSA name annotations.  */
>>>>
>>>> +  union vrp_info_type {
>>>> +    /* Pointer attributes used for alias analysis.  */
>>>> +    struct GTY ((tag ("TREE_SSA_PTR_INFO"))) ptr_info_def *ptr_info;
>>>> +    /* Value range attributes used for zero/sign extension
>>>> elimination.
>>>> */
>>>>
>>>> /* Value range information.  */
>>>>
>>>> +    struct GTY ((tag ("TREE_SSA_RANGE_INFO"))) range_info_def
>>>> *range_info;
>>>> +  } GTY ((desc ("%1.def_stmt && !POINTER_TYPE_P (TREE_TYPE
>>>> ((tree)&%1))"))) vrp;
>>>>
>>>> why do you need to test %1.def_stmt here?
>>>
>>>
>>>
>>> I have seen some tree_ssa_name with def_stmt NULL. Thats why I added
>>> this.
>>> Is that something that should never happen.
>>
>> It should never happen - they should have a GIMPLE_NOP.
>>
> 
> I am seeing def_stmt of NULL for TREE_NOTHROW node.
> debug_tree dumps the following in this case:
> 
> <ssa_name 0x2aaaabd89af8 nothrow var <var_decl 0x2aaaadb384c0 t>def_stmt
> 
>     version 11 in-free-list>

This is an invalid SSA name (in-free-list) that has been released.  You
shouldn't look at it at all.

Richard.

>> +void
>> +set_range_info (tree name, double_int min,
>> +                          double_int max, bool vr_range)
>>
>> you have some whitespace issues here (please properly use tabs)
>>
> 
> I will change it.
> 
>> +  /* Allocate if not available.  */
>> +  if (ri == NULL)
>> +    {
>> +      ri = ggc_alloc_cleared_range_info_def ();
>> +      mark_range_info_unknown (ri);
>>
>> that looks superfluous to me.
>>
>> +      SSA_NAME_RANGE_INFO (name) = ri;
>>
>> -  /* Pointer attributes used for alias analysis.  */
>> -  struct ptr_info_def *ptr_info;
>> +  /* Value range information.  */
>> +  union vrp_info_type {
>> +    /* Pointer attributes used for alias analysis.  */
>> +    struct GTY ((tag ("0"))) ptr_info_def *ptr_info;
>> +    /* Value range attributes used for zero/sign extension
>> elimination.  */
>> +    struct GTY ((tag ("1"))) range_info_def *range_info;
>> +  } GTY ((desc ("%1.def_stmt && !POINTER_TYPE_P (TREE_TYPE
>> ((tree)&%1))"))) vrp;
>>
>> please change vrp_info_type and vrp to other names - this is not vrp
>> specific info
>> after all, I suggest ssa_name_info_type and info.
>>
> 
> I will change this too.
>> The genric bits otherwise look ok to me, the VRP bits still look wrong
>> (see my
>> above question) and need explanation.
>>
>> Thanks,
>> Richard.
>>
>>>
>>> Thanks,
>>> Kugan
> 
> 
> Thanks,
> Kugan
>