This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]