This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Possible store motion tweak


While looking at unexpectedly-poor performance in a particular benchmark,
I noticed that an insn like:

    (set (mem:SI (symbol_ref "foo")) (const_int 0))

was enough to make gcse mark (mem:SI (symbol_ref "foo")) as invalid for
load/store motion.  The store has to meet the following condition, from
gcse.c:compute_ld_motion_mems():

		  /* Check for stores. Don't worry about aliased ones, they
		     will block any movement we might do later. We only care
		     about this exact pattern since those are the only
		     circumstance that we will ignore the aliasing info.  */
		  if (GET_CODE (dest) == MEM && simple_mem (dest))
		    {
		      ptr = ldst_entry (dest);

		      if (GET_CODE (src) != MEM
			  && GET_CODE (src) != ASM_OPERANDS
			  /* Check for REG manually since want_to_gcse_p
    note		     returns 0 for all REGs.  */
    ---->		  && (REG_P (src) || want_to_gcse_p (src)))
			ptr->stores = alloc_INSN_LIST (insn, ptr->stores);
		      else
			ptr->invalid = 1;
		    }

Not surprisingly, want_to_gcse_p returns false for const0_rtx.

So: is want_to_gcse_p really the right thing to check here?  Aren't we
trying to gcse DEST rather than SRC?

In addition to checking whether a value is too simple to be worthwhile,
want_to_gcse_p also checks whether a value can be assigned to an arbitrary
psuedo.  It seems like that's the main criterion here.  The following patch
splits it out into a separate function and makes the code above call it
directly.  This gave better results on a proprietary embedded benchmark.

To give a somewhat-contrived cut-down example:

    int i;
    void f (int n, unsigned short *x)
    {
      if (i < 10)
	i += n;
      x[i] = 1;
      i = 0;
    }

is compiled as follows before the patch:

        lw      $3,%gp_rel(i)($28)
        slt     $2,$3,10
        beq     $2,$0,$L2
        addu    $4,$3,$4

        sw      $4,%gp_rel(i)($28)
        move    $3,$4
$L2:
        sll     $2,$3,1
        addu    $2,$2,$5
        li      $3,1
        sw      $0,%gp_rel(i)($28)
        j       $31
        sh      $3,0($2)

After the patch, you get:

        lw      $3,%gp_rel(i)($28)
        slt     $2,$3,10
        bnel    $2,$0,$L2
        addu    $3,$3,$4

$L2:
        sll     $2,$3,1
        addu    $2,$2,$5
        li      $3,1
        sw      $0,%gp_rel(i)($28)
        j       $31
        sh      $3,0($2)

Bootstrapped & regression tested on mips64{,el}-linux-gnu.  OK to install?

Richard


	* gcse.c (can_assign_to_reg_p): New function, split out from...
	(want_to_gcse_p): ...here.
	(compute_ld_motion_mems): Use can_assign_to_reg_p to validate
	the rhs of a store.

Index: gcse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcse.c,v
retrieving revision 1.295
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.295 gcse.c
*** gcse.c	4 Mar 2004 16:28:47 -0000	1.295
--- gcse.c	17 Mar 2004 11:30:47 -0000
*************** static void hash_scan_set (rtx, rtx, str
*** 568,573 ****
--- 568,574 ----
  static void hash_scan_clobber (rtx, rtx, struct hash_table *);
  static void hash_scan_call (rtx, rtx, struct hash_table *);
  static int want_to_gcse_p (rtx);
+ static bool can_assign_to_reg_p (rtx);
  static bool gcse_constant_p (rtx);
  static int oprs_unchanged_p (rtx, rtx, int);
  static int oprs_anticipatable_p (rtx, rtx);
*************** static basic_block current_bb;
*** 1269,1281 ****
  /* See whether X, the source of a set, is something we want to consider for
     GCSE.  */
  
- static GTY(()) rtx test_insn;
  static int
  want_to_gcse_p (rtx x)
  {
-   int num_clobbers = 0;
-   int icode;
- 
    switch (GET_CODE (x))
      {
      case REG:
--- 1270,1278 ----
*************** want_to_gcse_p (rtx x)
*** 1288,1295 ****
        return 0;
  
      default:
!       break;
      }
  
    /* If this is a valid operand, we are OK.  If it's VOIDmode, we aren't.  */
    if (general_operand (x, GET_MODE (x)))
--- 1285,1305 ----
        return 0;
  
      default:
!       return can_assign_to_reg_p (x);
      }
+ }
+ 
+ /* Used internally by can_assign_to_reg_p.  */
+ 
+ static GTY(()) rtx test_insn;
+ 
+ /* Return true if we can assign X to a pseudo register.  */
+ 
+ static bool
+ can_assign_to_reg_p (rtx x)
+ {
+   int num_clobbers = 0;
+   int icode;
  
    /* If this is a valid operand, we are OK.  If it's VOIDmode, we aren't.  */
    if (general_operand (x, GET_MODE (x)))
*************** compute_ld_motion_mems (void)
*** 6838,6844 ****
  			  && GET_CODE (src) != ASM_OPERANDS
  			  /* Check for REG manually since want_to_gcse_p
  			     returns 0 for all REGs.  */
! 			  && (REG_P (src) || want_to_gcse_p (src)))
  			ptr->stores = alloc_INSN_LIST (insn, ptr->stores);
  		      else
  			ptr->invalid = 1;
--- 6848,6854 ----
  			  && GET_CODE (src) != ASM_OPERANDS
  			  /* Check for REG manually since want_to_gcse_p
  			     returns 0 for all REGs.  */
! 			  && can_assign_to_reg_p (src))
  			ptr->stores = alloc_INSN_LIST (insn, ptr->stores);
  		      else
  			ptr->invalid = 1;


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]