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

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
+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. 


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>


+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]