PATCH: Request for comments

[Jeffrey A Law <law at cygnus dot com>]

I'm not installing this patch just yet -- I want to sleep on it a little and
let folks here comment on it since I'm not 100% sure I've got the right
change.

Compiling the attached testcase on a sparc we generate incorrect code.

The problem is we increment "p" before calling memcpy for the structure
copy implied by *p++ = data_tmp.

>From what I can tell we have a queued post increment for the destination
address.  Something like this is on the pending chain:

(queued:SI (reg/v:SI 106)
    (nil)
    (nil)
    (set (reg/v:SI 106)
        (plus:SI (reg/v:SI 106)
            (const_int 404 [0x194])))
    (nil))



emit_block_move calls protect_from_queue.  In this particular case the post
increment has not happened, so protect_from_queue returns (reg:SI 106).

We then stuff (reg:SI 106) into an RTL_EXPR and save it away.  ie, we do not
use it (and if you read the comments in protect_from_queue, this is 
forbidden).
We begin expansion of the CALL_EXPR for the implicit memcpy call.  That
eventually calls precompute_register_parameters, which in turn calls 
emit_queue.

So we emit the increment of (reg:SI 106).

*Then* we extract (reg:SI 106) from the RTL_EXPR and copy it into an outgoing
hard parameter register.  And, gee, we get the incremented value which is
not what we wanted.

The easiest solution I see is to copy the result returned by protect_from_queue
into a new pseudo.  Then store the new pseudo into the RTL_EXPR.

That's precisely what this patch does.  Comments would be appreciated.  
Testcase
is attached after the patch (I'll cobble up an execution test for this later
after I sleep).


	* expr.c (emit_block_move): Copy arguments into fresh pseudos
	before expanding the CALL_EXPR for a memcpy.

Index: expr.c
===================================================================
RCS file: /egcs/carton/cvsfiles/egcs/gcc/expr.c,v
retrieving revision 1.144
diff -c -3 -p -r1.144 expr.c
*** expr.c	1999/05/17 07:21:14	1.144
--- expr.c	1999/06/25 16:09:04
*************** emit_block_move (x, y, size, align)
*** 1661,1666 ****
--- 1661,1669 ----
      move_by_pieces (x, y, INTVAL (size), align);
    else
      {
+ #ifdef TARGET_MEM_FUNCTIONS
+       rtx new_x, new_y, new_size;
+ #endif
        /* Try the most limited insn first, because there's no point
  	 including more than one in the machine description unless
  	 the more limited one has some advantage.  */
*************** emit_block_move (x, y, size, align)
*** 1742,1761 ****
  	  pop_obstacks ();
  	}
  
        /* We need to make an argument list for the function call. 
  
  	 memcpy has three arguments, the first two are void * addresses and
  	 the last is a size_t byte count for the copy.  */
        arg_list
  	= build_tree_list (NULL_TREE,
! 			    make_tree (build_pointer_type (void_type_node),
! 				       XEXP (x, 0)));
        TREE_CHAIN (arg_list)
  	= build_tree_list (NULL_TREE,
  			   make_tree (build_pointer_type (void_type_node),
! 				      XEXP (y, 0)));
        TREE_CHAIN (TREE_CHAIN (arg_list))
! 	 = build_tree_list (NULL_TREE, make_tree (sizetype, size));
        TREE_CHAIN (TREE_CHAIN (TREE_CHAIN (arg_list))) = NULL_TREE;
  
        /* Now we have to build up the CALL_EXPR itself.  */
--- 1745,1788 ----
  	  pop_obstacks ();
  	}
  
+       /* X, Y, or SIZE have been passed through protect_from_queue.
+ 
+ 	 It is unsafe to save the value generated by protect_from_queue
+ 	 and reuse it later.  Consider what happens if emit_queue is
+ 	 called before the return value from protect_from_queue is used.
+ 
+ 	 Expansion of the CALL_EXPR below will call emit_queue before
+ 	 we are finished emitting RTL for argument setup.  So if we are
+ 	 not careful we could get the wrong value for an argument.
+ 
+ 	 To avoid this problem we go ahead and emit code to copy X, Y &
+ 	 SIZE into new pseudos.  We can then place those new pseudos
+ 	 into an RTL_EXPR and use them later, even after a call to
+ 	 emit_queue.
+ 
+ 	 ??? Using force operand may not be necessary here, especially
+ 	 for X & Y, but why take chances.  */
+       new_x = gen_reg_rtx (Pmode);
+       new_y = gen_reg_rtx (Pmode);
+       new_size = gen_reg_rtx (TYPE_MODE (sizetype));
+       emit_move_insn (new_x, force_operand (XEXP (x, 0), NULL_RTX));
+       emit_move_insn (new_y, force_operand (XEXP (y, 0), NULL_RTX));
+       emit_move_insn (new_size, force_operand (new_size, NULL_RTX));
+ 
        /* We need to make an argument list for the function call. 
  
  	 memcpy has three arguments, the first two are void * addresses and
  	 the last is a size_t byte count for the copy.  */
        arg_list
  	= build_tree_list (NULL_TREE,
! 			   make_tree (build_pointer_type (void_type_node),
! 				      new_x));
        TREE_CHAIN (arg_list)
  	= build_tree_list (NULL_TREE,
  			   make_tree (build_pointer_type (void_type_node),
! 				      new_y));
        TREE_CHAIN (TREE_CHAIN (arg_list))
! 	 = build_tree_list (NULL_TREE, make_tree (sizetype, new_size));
        TREE_CHAIN (TREE_CHAIN (TREE_CHAIN (arg_list))) = NULL_TREE;
  
        /* Now we have to build up the CALL_EXPR itself.  */




Testcase:

struct {
    long sqlcode;
} sqlca;


struct data_record {
    int dummy;
    int a[100];
} *data_ptr, data_tmp;


void
load_data() {
    struct data_record *p;
    int num = num_records();

    data_ptr = malloc(num * sizeof(struct data_record));
    memset(data_ptr, 0xaa, num * sizeof(struct data_record));

    fetch();
    p = data_ptr;
    while (sqlca.sqlcode == 0) {
        *p++ = data_tmp;
        fetch();
    }
}