[ping][PATCH][1 of 2] Add value range info to SSA_NAME for zero sign extension elimination in RTL
Kugan
kugan.vivekanandarajah@linaro.org
Mon Sep 9 00:15:00 GMT 2013
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.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
[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.
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.
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.
What is your preferred solution please.
>>>> 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:
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
>>
>
More information about the Gcc-patches
mailing list