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: [patch] Miscelaneous ivopts improvements (1 of 3)


On Mon, 7 Feb 2005, Zdenek Dvorak wrote:
>
> 	* tree-ssa-loop-ivopts.c (determine_base_object): Ignore casts.
> 	(strip_offset): Handle addresses.
> 	(add_address_candidates): Use strip_offset.
> 	(difference_cost): Reflect strip_offset change.


I think your rewrite of this patch has introduced an inefficiency
that undermines some of the underlying logic.

> + static tree
> + strip_offset (tree expr, bool inside_addr, unsigned HOST_WIDE_INT *offset)
> + {
> +   tree op0 = NULL_TREE, op1 = NULL_TREE, step;
> +   enum tree_code code;
> +   tree type, orig_type = TREE_TYPE (expr);
> +   unsigned HOST_WIDE_INT off0, off1, st;
> +
> +   STRIP_NOPS (expr);
> +   type = TREE_TYPE (expr);
> +   code = TREE_CODE (expr);
> +   *offset = 0;
> +
> +   switch (code)
> +     {
> +     case INTEGER_CST:
> +       if (!cst_and_fits_in_hwi (expr))
> + 	return fold_convert (orig_type, expr);

You're now careful to always return fold_convert (orig_type, expr) to
ensure that the type of the returned tree matches the original.  However,
I think you really want an orig_expr variable that holds the value of
expr prior to STRIP_NOPS and return this if there's nothing to do.
Otherwise, if STRIP_NOPS removes a NOP_EXPR, we'll simply create another
when we call fold_convert, but upon return from a recursive call, the
tree we passed in no longer compares pointer equal to the tree we get
back, so we end up deep copying a tree needlessly.


And one question about this bit:

> +     case PLUS_EXPR:
> +     case MINUS_EXPR:
> +       op0 = TREE_OPERAND (expr, 0);
> +       op1 = TREE_OPERAND (expr, 1);
> +
> +       op0 = strip_offset (op0, false, &off0);
> +       op1 = strip_offset (op1, false, &off1);
> +
> +       *offset = (code == PLUS_EXPR ? off0 + off1 : off0 - off1);
> +       if (op0 != TREE_OPERAND (expr, 0)
> + 	  || op1 != TREE_OPERAND (expr, 1))

Why not call "expr = fold (build2 (code, type, op0, op1));" at this
point, rather than duplicate

	x + 0  ->  x
	0 + x  ->  x
	x - 0  ->  x
	0 - x  -> -x

For example, you miss all of the "x - x -> 0", and "x + x -> 2*x"
transformations and simplifications.


Could you bootstrap, regtest and repost a version that addresses the
above issues?

Roger
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]