On Thu, 6 Oct 2011, William J. Schmidt wrote:
> On Thu, 2011-10-06 at 12:13 +0200, Richard Guenther wrote:
> > People have already commented on pieces, so I'm looking only
> > at the tree-ssa-reassoc.c pieces (did you consider piggy-backing
> > on IVOPTs instead? The idea is to expose additional CSE
> > opportunities, right? So it's sort-of a strength-reduction
> > optimization on scalar code (classically strength reduction
> > in loops transforms for (i) { z = i*x; } to z = 0; for (i) { z += x }).
> > That might be worth in general, even for non-address cases.
> > So - if you rename that thing to tree-ssa-strength-reduce.c you
> > can get away without piggy-backing on anything ;) If you
> > structure it to detect a strength reduction opportunity
> > (thus, you'd need to match two/multiple of the patterns at the same time)
> > that would be a bonus ... generalizing it a little bit would be
> > another.
>
> These are all good ideas. I will think about casting this as a more
> general strength reduction over extended basic blocks outside of loops.
> First I'll put together some simple tests to see whether we're currently
> missing some non-address opportunities.
>
> <snip>
>
> > > + mult_op0 = TREE_OPERAND (offset, 0);
> > > + mult_op1 = TREE_OPERAND (offset, 1);
> > > +
> > > + if (TREE_CODE (mult_op0) != PLUS_EXPR
> > > + || TREE_CODE (mult_op1) != INTEGER_CST
> > > + || TREE_CODE (TREE_OPERAND (mult_op0, 1)) != INTEGER_CST)
> > > + return NULL_TREE;
> > > +
> > > + t1 = TREE_OPERAND (base, 0);
> > > + t2 = TREE_OPERAND (mult_op0, 0);
> > > + c1 = TREE_INT_CST_LOW (TREE_OPERAND (base, 1));
> > > + c2 = TREE_INT_CST_LOW (TREE_OPERAND (mult_op0, 1));
> > > + c3 = TREE_INT_CST_LOW (mult_op1);
> >
> > Before accessing TREE_INT_CST_LOW you need to make sure the
> > constants fit into a HWI using host_integerp () (which
> > conveniently includes the check for INTEGER_CST).
> >
> > Note that you need to sign-extend the MEM_REF offset,
> > thus use mem_ref_offset (base).low instead of
> > TREE_INT_CST_LOW (TREE_OPERAND (base, 1)). Might be worth
> > to add a testcase with negative offset ;)
>
> D'oh! >.<
>
> >
> > > + c4 = bitpos / BITS_PER_UNIT;
> > > + c = c1 + c2 * c3 + c4;
> >
> > And you don't know whether this operation overflows. Thus it's
> > probably easiest to use double_ints instead of HOST_WIDE_INTs
> > in all of the code.
>
> OK, thanks, will do.
>
> <snip>
>
> >
> > > + /* Determine whether the expression can be represented with base and
> > > + offset components. */
> > > + base = get_inner_reference (*expr, &bitsize, &bitpos, &offset, &mode,
> > > + &unsignedp, &volatilep, false);
> > > + if (!base || !offset)
> > > + return false;
> > > +
> > > + /* Look for a restructuring opportunity. */
> > > + if ((mem_ref = restructure_base_and_offset (expr, gsi, base,
> > > + offset, bitpos)) == NULL_TREE)
> > > + return false;
> >
> > What I'm missing is a check whether the old address computation stmts
> > will be dead after the transform.
>
> Hm, not quite sure what to do here. Prior to the transformation I'll
> have an assignment with something like:
>
> ARRAY_REF (COMPONENT_REF (MEM_REF (Ta, Cb), FIELD_DECL c), Td)
>
> on LHS or RHS. Ta and Td will be part of the replacement. What should
> I be checking for?
Doh, I thought you were matching gimple stmts that do the address
computation. But now I see you are matching the tree returned from
get_inner_reference. So no need to check anything for that case.
But that keeps me wondering what you'll do if the accesses were
all pointer arithmetic, not arrays. Thus,
extern void foo (int, int, int);
void
f (int *p, unsigned int n)
{
foo (p[n], p[n+64], p[n+128]);
}
wouldn't that have the same issue and you wouldn't handle it?
Richard.
