Re: [PATCH] Address lowering [1/3] Main patch

["William J. Schmidt" <wschmidt at linux dot vnet dot ibm dot com>]

On Mon, 2011-07-04 at 17:30 +0200, Michael Matz wrote:
> Hi,
> 
> On Mon, 4 Jul 2011, Richard Guenther wrote:
> 
> > I still do not like the implementation of yet another CSE machinery
> > given that we already have two.
> 
> From reading it it really seems to be a normal block-local CSE, without 
> anything fancy.  Hence, moving the pass just a little earlier (before 
> pass_vrp/pass_dominator) should already provide for all optimizations.  If 
> not those should be improved.
> 
> I see that it is used for also getting rid of the zero-offset statements 
> in case non-zero-offsets follow.  I think that's generally worthwhile so 
> probably should be done in one of the above optimizers.

OK, thanks for the suggestion.  I can certainly look into this.

> 
> You handle NOP_EXPR different from CONVERT_EXPR.  The middle-end doesn't 
> distinguish between them (yes, ignore the comment about one generating 
> code, the other not).
> 

Ah, thanks, good to know.

> Your check for small types:
> 
> +         if (TYPE_MODE (TREE_TYPE (TREE_OPERAND (expr, 0))) == SImode)
> +           ref_found = true;
> 
> You probably want != BLKmode .
> 

OK.

> +  if (changed && is_zero_offset_ref (gimple_assign_lhs (stmt)))
> +    VEC_safe_push (gimple, heap, zero_offset_refs, stmt);
> +
> +  rhs1 = gimple_assign_rhs1_ptr (stmt);
> +  rhs_changed = tree_ssa_lower_addr_tree (rhs1, gsi, speed, false);
> +
> +  /* Record zero-offset mem_refs on the RHS.  */
> +  if (rhs_changed && is_zero_offset_ref (gimple_assign_rhs1 (stmt)))
> +    VEC_safe_push (gimple, heap, zero_offset_refs, stmt);
> 
> This possibly adds stmt twice to zero_offset_refs.  Do you really want 
> this?
> 

Hm, I didn't think it was (currently) possible for a gimple statement to
have a mem-ref on both RHS and LHS.  Is that incorrect?  This is easily
changed if so, or if the possibility should be left open for the future.

> 
> Ciao,
> Michael.

Thanks,
Bill