Re: [patch] TARGET_MEM_REF (resent)

[Steven Bosscher <stevenb at suse dot de>]

On Thursday 31 March 2005 22:54, Zdenek Dvorak wrote:
> Hello,
>
> during playing with disabling the old loop optimizer, I implemented
> a few improvements over the earlier version of the patch
> (http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00450.html):
>
> -- the functions that produce TARGET_MEM_REFs are a bit more clever when
>    recognizing the more complex forms of addresses.
> -- if constants are propagated to TARGET_MEM_REFs, folding is performed
>    (they are moved to TMR_OFFSET if the address remains valid).
>
> Bootstrapped & regtested on i686, x86_x64 and ppc.

I am really starting to like this patch.  I understand you originally
worked on this so that loop.c can go away, but your patch also reduces
the number of optimizations CSE does by two orders of magnitude on ppc
and amd64.  Hubba!  I only gave the patch a brief look, and have a few
minor comments on it:

> 	* tree.h (REF_ORIGINAL): Expect TARGET_MEM_REF as an argument.

IMHO it'd be nice if you can rename REF_ORIGINAL to TMR_REF_ORIGINAL,
or something like that.  And would not using TREE_CHAIN for it be an
option, please? :-/

> *************** expand_expr_real_1 (tree exp, rtx target
> *** 6880,6885 ****
> --- 6876,6893 ----
>   	return temp;
>         }
>
> +     case TARGET_MEM_REF:
> +       {
> + 	struct mem_address addr;
> +
> + 	get_address_description (exp, &addr);
> + 	op0 = addr_for_mem_ref (&addr, true);
> + 	op0 = memory_address (mode, op0);
> + 	temp = gen_rtx_MEM (mode, op0);
> + 	set_mem_attributes (temp, REF_ORIGINAL (exp), 0);
> +       }
> +       return temp;
> +
>       case ARRAY_REF:
>
>         {

I suppose you know for sure that the TARGET_MEM_REF will map to some
machine addressing mode, right?  If so, can it be verified here that
this indeed happened somehow?

> *************** tree_could_trap_p (tree expr)
> *** 1739,1744 ****
> --- 1739,1750 ----
>    restart:
>     switch (code)
>       {
> +     case TARGET_MEM_REF:
> +       /* For MEM_REFs use the information based on the original reference.  */
                 ^
                 TARGET_MEM_REFs

> + /* Annotation for TARGET_MEM_REFs.  */
> +
> + struct mem_ref_ann_d GTY(())
> + {
> +   struct tree_ann_common_d common;
> +
> +   /* Name tag, type tag, or base object, whichever is applicable
> +      for the reference from that the TARGET_MEM_REF was created.  */
> +   tree tag;
> + };
> +

Makes me wonder, can you use this tag field instead of REF_ORIGINAL?

> + /* A "template" for memory address, used to determine whether the address is
> +    valid for mode.  */ 
> +
> + struct mem_addr_template GTY (())
> + {
> +   rtx ref;			/* The template.  */
> +   rtx * GTY ((skip)) step_p;	/* The point in template where the step should be
> + 				   filled in.  */ 
> +   rtx * GTY ((skip)) off_p;	/* The point in template where the offset should
> + 				   be filled in.  */ 
> + };
> +
> + /* The templates.  */
> +
> + static GTY (()) struct mem_addr_template templates[32];

Whee, magic number 32.  Could use a comment.

> + tree
> + create_mem_ref (block_stmt_iterator *bsi, tree type, tree addr)
> + {
(...)
> +   gcc_assert (parts.symbol == NULL_TREE);
> +   gcc_assert (parts.base == NULL_TREE);
> +   gcc_assert (!parts.step || integer_onep (parts.step));
> +   gcc_assert (!parts.offset || integer_zerop (parts.offset));
> +
> +   /* Things won't get any simpler.  */
> +   gcc_unreachable ();
> + }

This looks fishy.  First assert things, then unconditionally die?

> + @item TARGET_MEM_REF
> + These nodes represent memory accesses whose address directly map to
> + an addressing mode of the target architecture.  The first argument
> + is @code{TMR_SYMBOL} and must be a @code{VAR_DECL} of an object with
> + a fixed address.  The second argument is @code{TMR_BASE} and the
> + third one is @code{TMR_INDEX}.  The fourth argument is
> + @code{TMR_STEP} and must be an @code{INTEGER_CST}.  The fifth
> + argument is @code{TMR_OFFSET} and must be an @code{INTEGER_CST}.
> + Any of the arguments may be NULL if the appropriate component
> + does not appear in the address.  Address of the @code{TARGET_MEM_REF}
> + is determined in the following way.
> +
> + @smallexample
> + &TMR_SYMBOL + TMR_BASE + TMR_INDEX * TMR_STEP + TMR_OFFSET
> + @end smallexample
> +

REF_ORIGINAL should probably be mentioned here, too.  It is currently
not documented at all.

> Index: testsuite/gcc.dg/20050105-1.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/testsuite/gcc.dg/20050105-1.c,v
> retrieving revision 1.1
> diff -c -3 -p -r1.1 20050105-1.c
> *** testsuite/gcc.dg/20050105-1.c	7 Jan 2005 09:04:01 -0000	1.1
> --- testsuite/gcc.dg/20050105-1.c	31 Mar 2005 20:45:22 -0000
> ***************
> *** 1,6 ****
>   /* PR rtl-optimization/18861 */
>   /* { dg-do compile } */
> ! /* { dg-options "-O2 -floop-optimize2" } */
>
>   extern void abort (void);
>
> --- 1,6 ----
>   /* PR rtl-optimization/18861 */
>   /* { dg-do compile } */
> ! /* { dg-options "-O2" } */
>
>   extern void abort (void);

Hmmm...??  This change is not in the ChangeLog.

As a final remark, in your original posting of this patch[1]  you had
SPEC numbers, with rth later called "sort of underwhelming".  You said
that the old loop optimizer, together with CSE and combine sometimes,
fix up the poor addressing mode choices GCC makes in 'expand'.  Maybe
you could post SPEC numbers with the old loop optimizer disabled (and
maybe even CSE path following as well)?  That could clearify why this
patch is a Good Thing.

Gr.
Steven

[1] http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00272.html
[2] http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00802.html