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 09/09/13 19:01, Richard Biener wrote:

On Mon, Sep 9, 2013 at 1:09 AM, Kugan <kugan.vivekanandarajah@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.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.


If it's a sign-changing conversion it can be surely wrong.
It is not sign-changing conversion. Rather, when we have rhs expression value which is VR_VARYING it is set to [0, +INF]
i.e, in extract_range_from_assignment, if the value range is VR_VARYING, follwing is done 
 if (vr->type == VR_VARYING)
     extract_range_basic (vr, stmt);
In extract_range_basic (when the value range is varying), when the following code executes, it changes VR_VARYING to [0, +INF], 

 if (INTEGRAL_TYPE_P (type)
       && gimple_stmt_nonnegative_warnv_p (stmt, &sop))
     set_value_range_to_nonnegative (vr, type,
                                     sop || stmt_overflow_infinity (stmt));
This will happen only when we have VR_VARYING for the rhs expression. This is wrong from zero/sign extension elimination point of view as we cant rely on this converted value range.
Currently I am leaving this as varying so that we can decide whether to eliminate the zero/sign extension. This is not completely wrong. 

unsigned short s;
s.1_3 = (short unsigned int) l.0_2;
l.0_2: VARYING
s.1_3: [0, +INF]

Similarly (extracted form a testcase)

unsigned char _4;
unsigned char _2;
unsigned char _5;

  _5 = _4 + _2;
value range extracted for expression (_4 + _2) extract_range_from_binary_expr is VARYING and _5 has value range [0 +INF] or [0, 255] after set_value_range_to_nonnegative is done. 



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.



I hope I have explained it better this time.


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.



Could you please suggest the preferred way now.


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