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

[Richard Biener <rguenther at suse dot de>]

On Mon, 16 Sep 2013, Kugan wrote:

> Hi,
> 
> Updated the patch to the latest changes in trunk that splits tree.h. I also
> noticed an error in printing double_int and fixed it.
> 
> Is this OK?

       print_gimple_stmt (dump_file, stmt, 0,
-                        TDF_SLIM | (dump_flags & TDF_LINENO));
+                        TDF_SLIM | TDF_RANGE | (dump_flags & 
TDF_LINENO));

this should be (dump_flags & (TDF_LINENO|TDF_RANGE)) do not always
dump range info.  I'd have simply re-used TDF_ALIAS (and interpret
it as SSA annotation info), adding -range in dump file modifiers
is ok with me.

+static void
+print_double_int (pretty_printer *buffer, double_int cst)
+{
+  tree node = double_int_to_tree (integer_type_node, cst);
+  if (TREE_INT_CST_HIGH (node) == 0)
+    pp_printf (buffer, HOST_WIDE_INT_PRINT_UNSIGNED, TREE_INT_CST_LOW 
(node));
+  else if (TREE_INT_CST_HIGH (node) == -1
+           && TREE_INT_CST_LOW (node) != 0)
+    pp_printf (buffer, "-" HOST_WIDE_INT_PRINT_UNSIGNED,
+               -TREE_INT_CST_LOW (node));
+  else
+    sprintf (pp_buffer (buffer)->digit_buffer,
+             HOST_WIDE_INT_PRINT_DOUBLE_HEX,
+             (unsigned HOST_WIDE_INT) TREE_INT_CST_HIGH (node),
+             (unsigned HOST_WIDE_INT) TREE_INT_CST_LOW (node));

using sprintf here looks like a layering violation to me.  You
probably want to factor out code from the INTEGER_CST handling
of tree-pretty-print.c:dump_generic_node into a pp_double_int
function in pretty-print.[ch] instead.

@@ -1628,6 +1647,27 @@ dump_gimple_phi (pretty_printer *buffer, gimple 
phi, int spc, int flags)
       pp_string (buffer, "# ");
     }

+  if ((flags & TDF_RANGE)
+      && !POINTER_TYPE_P (TREE_TYPE (lhs))
+      && SSA_NAME_RANGE_INFO (lhs))
+    {
+      double_int min, max;
+      value_range_type range_type;

I realize the scheme is pre-existing but can you try factoring
out the dumping of SSA_NAME_PTR_INFO / SSA_NAME_RANGE_INFO into
a separate routine that can be shared by dump_gimple_phi and
pp_gimple_stmt_1?

+get_range_info (tree name, double_int &min, double_int &max,
+                enum value_range_type &range_type)
+{
+  gcc_assert (!POINTER_TYPE_P (TREE_TYPE (name)));
+  gcc_assert (TREE_CODE (name) == SSA_NAME);
+  range_info_def *ri = SSA_NAME_RANGE_INFO (name);

the TREE_CODE (name) == SSA_NAME assert is redundant with the
tree-checking performed by SSA_NAME_RANGE_INFO.  Likewise in
the other functions.

+void
+get_range_info (tree name, double_int &min, double_int &max,
+                enum value_range_type &range_type)

I'm not sure we want to use references.  Well - first time.

+  /* If min > max, it is  VR_ANTI_RANGE.  */
+  if (ri->min.scmp (ri->max) == 1)
+    {

I think that's wrong and needs to be conditional on TYPE_UNSIGNED
of the SSA name.

+          else if (vr_value[i]->type == VR_ANTI_RANGE)
+            {
+              /* VR_ANTI_RANGE ~[min, max] is encoded compactly as
+                 [max + 1, min - 1] without additional attributes.
+                 When min value > max value, we know that it is
+                 VR_ANTI_RANGE; it is VR_RANGE othewise.  */
+              set_range_info (name,
+                              tree_to_double_int (vr_value[i]->max)
+                              + double_int_one,
+                              tree_to_double_int (vr_value[i]->min)
+                              - double_int_one);

there is a complication for when max + 1 or min - 1 overflow - those
should be non-canonical ranges I think, but double-check this
(check set_and_canonicalize_value_range).

+/* Type of value ranges.  See value_range_d In tree-vrp.c for a
+   description of these types.  */
+enum value_range_type { VR_UNDEFINED, VR_RANGE, VR_ANTI_RANGE, VR_VARYING 
};
+
+/* Sets the value range to SSA.  */
+void set_range_info (tree ssa, double_int min, double_int max);
+/* Gets the value range from SSA.  */
+void get_range_info (tree name, double_int &min, double_int &max,
+                     enum value_range_type &range_type);

put these into tree-ssanames.h please, likewise struct GTY (()) 
range_info_def - this is where the ptr-info stuff went very recently.

Thanks,
Richard.



> Thanks,
> Kugan
> 
> 
> +2013-09-12  Kugan Vivekanandarajah  <kuganv@linaro.org>
> +
> +	* cfgexpand.c (maybe_dump_rtl_for_gimple_stmt) : Add range to dump.
> +	* gimple-pretty-print.c (print_double_int) : New function.
> +	* gimple-pretty-print.c (dump_gimple_phi) : Dump range info.
> +	* (pp_gimple_stmt_1) : Likewise.
> +	* tree-ssa-alias.c (dump_alias_info) : Check pointer type.
> +	* tree-ssa-copy.c (fini_copy_prop) : Check pointer type and copy
> +	range info.
> +	* tree-ssanames.c (make_ssa_name_fn) : Check pointer type in
> +	initialize.
> +	* (set_range_info) : New function.
> +	* (get_range_info) : Likewise.
> +	* (duplicate_ssa_name_range_info) : Likewise.
> +	* (duplicate_ssa_name_fn) : Check pointer type and call correct
> +	duplicate function.
> +	* tree-vrp.c (vrp_finalize): Call set_range_info to upddate
> +	value range of SSA_NAMEs.
> +	* tree.h (SSA_NAME_PTR_INFO) : changed to access via union
> +	* tree.h (SSA_NAME_RANGE_INFO) : New macro
> +
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend