Re: [patch] Miscelaneous ivopts improvements (1 of 3)

[Zdenek Dvorak <rakdver at atrey dot karlin dot mff dot cuni dot cz>]

Hello,

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

right.

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

to avoid building a new tree, and an useless call to fold -- stripping
the constant offset should not reveal any new opportunities for folding
(at least not significant ones);  in particular the cases you mention
should never (or almost never) occur.

Zdenek