On Mon, 17 Jun 2013, Kugan wrote:
> Can you please help to review this patch? Richard reviewed the original patch
> and asked it to be split into two parts. Also, he wanted a review from RTL
> maintainer for the RTL changes.
>
> Thanks,
> Kugan
>
> On 03/06/13 11:43, Kugan wrote:
> > Hi,
> >
> > This patch adds value range information to tree SSA_NAME during Value
> > Range Propagation (VRP) pass in preparation to removes some of the
> > redundant sign/zero extensions during RTL expansion.
> >
> > This is based on the original patch posted in
> > http://gcc.gnu.org/ml/gcc-patches/2013-05/msg00610.html and addresses
> > the review comments of Richard Biener.
> >
> > Tested on X86_64 and ARM.
> >
> > I would like review comments on this.
> >
> > Thanks,
> > Kugan
> >
> >
> > +2013-06-03 Kugan Vivekanandarajah <kug...@linaro.org>
> > +
> > + * gcc/gcc/tree-flow.h: Declared structure range_info_def and function
> > + definition for mark_range_info_unknown.
> > + * gcc/tree-ssa-alias.c (dump_alias_info) : Check pointer type
> > + * gcc/tree-ssanames.c (make_ssa_name_fn) : Check pointer type in
> > + initialize.
> > + * (mark_range_info_unknown) : New function.
> > + * (duplicate_ssa_name_range_info) : Likewise.
> > + * (duplicate_ssa_name_fn) : Check pointer type and call correct
> > + duplicate function.
> > + * gcc/tree-vrp.c (extract_exp_value_range): New function.
> > + * (simplify_stmt_using_ranges): Call extract_exp_value_range and
> > + tree_ssa_set_value_range.
> > + * gcc/tree.c (tree_ssa_set_value_range): New function.
> > + * gcc/tree.h (SSA_NAME_PTR_INFO) : changed to access via union
> > + * gcc/tree.h (SSA_NAME_RANGE_INFO) : New macro
These go to gcc/ChangeLog and thus do not need the gcc/ prefix.
+/* Value range information for SSA_NAMEs representing non-pointer
variables. */
+
+struct GTY (()) range_info_def {
+ /* Set to true if VR_RANGE and false if VR_ANTI_RANGE. */
+ bool vr_range;
+ /* Minmum for value range. */
+ double_int min;
+ /* Maximum for value range. */
+ double_int max;
+ /* Set to true if range is valid. */
+ bool valid;
+};
this way you waste a lot of padding, so please move the two bool
members next to each other.
+/* 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);
}
that is, transfer directly from the lattice.
+/* Set zero/sign extension redundant for ssa def stmt. */
+
+void
+tree_ssa_set_value_range (tree ssa, double_int min,
+ double_int max, bool vr_range)
+{
The comment needs updating. Also this doesn't belong in tree.c
but in tree-ssanames.c with a more suitable name like
set_range_info ().
+/* The garbage collector needs to know the interpretation of the
+ value range info in the tree_ssa_name. */
+#define TREE_SSA_PTR_INFO (0)
+#define TREE_SSA_RANGE_INFO (1)
no need for those, just use GTY ((tag ("0")) and "1".
+ /* Value range information. */
/* 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?
Please add a way to dump the associated range information (I'm fine
doing it at the time and with the same flags we dump SSA_NAME_PTR_INFO,
see gimple-pretty-print.c.
Sorry for taking so long to review this again.
Thanks,
Richard.
