On Mon, Sep 9, 2013 at 1:09 AM, Kugan <kugan.vivekanandara...@linaro.org> wrote:
>
> On 06/09/13 16:16, Richard Biener wrote:
>>
>> 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
>
>
> [snip]
>
>
>>>>>>
>>>>>> 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.
>>
>
> Yes, tree SSA_NAME should have it's correct value range. But, assigning rhs
> expression's value range is not totally wrong , it is just that it can be
> conservative value range (please correct me if I am wrong here) in few
> cases, as it can have wider range.
If it's a sign-changing conversion it can be surely wrong.
> I can use the rhs value range in the above case. We can also eliminate
> redundant zero/sign extensions for gimple binary and ternary stmts. In this
> case we will have to calculate the value range. We will have to reuse these
> logic in tree-vrp.
I fail to see the issue given no concrete example.
> Other option is to add another attribute in range_info_t to indicate if
> set_value_range_to_nonnegative is used in value range extraction.
Why should the result of this not be accurately represented in the lattice?
> What is your preferred solution please.
I don't know because I do not understand the problem at hand.
>>>>> 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.
>
>
> This is actually happening in garbage collection. If i remove the check, I
> get the following:
Ah, ok. Well, that shows that the field is not properly zeroed at SSA name
release time. Or that the garbage collector visits the ptr_info (your .vrp)
even if it is NULL. You can look at the gtype-desc.c code and the
way the callers are wrapped via gtype-desc.h.
I suspect that the right answer is to instead do
GTY ((desc (%1.ptr_info && ...)))
that is, make sure the pointer is not NULL. Or push the union into the
pointer target instead.
Do others have a better solution? The issue is that the descriptor for
the GT machinery is not valid if the "field" (either of the pointers in the
union) is NULL. But then it wouldn't matter anyway. The GTY machinery
doesn't seem to handle this special-case (all-pointers in the union).
Richard.
> make[2]: *** [_mulsc3.o] Error 1
> 0x97755f crash_signal
> /home/kugan/work/sources/gcc-fsf/test/gcc/toplev.c:335
> 0x55f7d1 gt_ggc_mx_lang_tree_node(void*)
> ./gt-c-c-decl.h:531
> 0x802e36 gt_ggc_mx<tree_node*>
> /home/kugan/work/sources/gcc-fsf/test/gcc/vec.h:1152
> 0x802e36 gt_ggc_mx_vec_tree_va_gc_(void*)
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:1205
> 0x8081ed gt_ggc_mx_gimple_df(void*)
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:1066
> 0x80825f gt_ggc_mx_function
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:1280
> 0x80825f gt_ggc_mx_function(void*)
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:1272
> 0x55f692 gt_ggc_mx_lang_tree_node(void*)
> ./gt-c-c-decl.h:389
> 0x807dd5 gt_ggc_mx_symtab_node_def(void*)
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:709
> 0x807fb0 gt_ggc_m_P15symtab_node_def4htab(void*)
>
> /home/kugan/work/builds/gcc-fsf-test/obj-arm-none-linux-gnueabi/gcc1/gcc/gtype-desc.c:2928
> 0x7b7915 ggc_mark_root_tab
> /home/kugan/work/sources/gcc-fsf/test/gcc/ggc-common.c:133
> 0x7b7c60 ggc_mark_roots()
> /home/kugan/work/sources/gcc-fsf/test/gcc/ggc-common.c:152
> 0x615669 ggc_collect()
> /home/kugan/work/sources/gcc-fsf/test/gcc/ggc-page.c:2077
>
>
>
>
> Thanks a lot.
>
> Kugan
>
>
>> 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
>>>
>>
>
