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.vivekanandara...@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
>
