GCSE considers read only memory clobbered by function calls.

GCSE considers read only memory clobbered by function calls.

[Mostafa Hagog]

It appears that GCSE considers "read only memory" as call clobbered, which
is not the case in CSE. I have took the test for read-only memory from CSE
and add it to GCSE where we compute the transparency. Here is a patch that
does so. This patch makes gcse eliminate redundant loads after stores for
the following example. The difference is seen when we compile with the
options:
"-O3 --param max-gcse-passes=3" with/without "-fgcse-las" (on a
powerpc-linux target). If this looks a reasonable change I will
regression-test and bootstrap the patch and ask for commit.

Example:

#define CALL_FPTR(fptr) (*fptr)
#define MY_FOO_CHECK() if (my_foo_var) my_foo_func()

int my_foo_var;

struct my_foo_struct {
    int my_dummy_field;
    int *(*ppaddr)(int);
};

struct my_foo_struct *my_foo_record;

int my_main_foo(int n)
{
    while ((my_foo_record = CALL_FPTR(my_foo_record->ppaddr)(n))) {
        MY_FOO_CHECK();
    }

    return 0;
}

The patch:

2005-05-09 Mostafa Hagog <mustafa@il.ibm.com>

      * gcse.c (compute_transp): use MEM_READONLY_P in case of MEM.

Index: gcse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcse.c,v
retrieving revision 1.340
diff -c -p -r1.340 gcse.c
*** gcse.c      23 Apr 2005 21:27:44 -0000      1.340
--- gcse.c      5 May 2005 12:54:04 -0000
*************** compute_transp (rtx x, int indx, sbitmap
*** 2470,2479 ****
           do any list walking for them.  */
        EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
          {
!           if (set_p)
!             SET_BIT (bmap[bb_index], indx);
!           else
!             RESET_BIT (bmap[bb_index], indx);
          }

        /* Now iterate over the blocks which have memory modifications
--- 2470,2482 ----
           do any list walking for them.  */
        EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
          {
!             if (! MEM_READONLY_P (x))
!               {
!                 if (set_p)
!                   SET_BIT (bmap[bb_index], indx);
!                 else
!                   RESET_BIT (bmap[bb_index], indx);
!               }
          }

Re: GCSE considers read only memory clobbered by function calls.

[Jeffrey A Law]

On Mon, 2005-05-09 at 17:45 +0300, Mostafa Hagog wrote:
> 
> 
> 
> It appears that GCSE considers "read only memory" as call clobbered, which
> is not the case in CSE. I have took the test for read-only memory from CSE
> and add it to GCSE where we compute the transparency. Here is a patch that
> does so. This patch makes gcse eliminate redundant loads after stores for
> the following example. The difference is seen when we compile with the
> options:
> "-O3 --param max-gcse-passes=3" with/without "-fgcse-las" (on a
> powerpc-linux target). If this looks a reasonable change I will
> regression-test and bootstrap the patch and ask for commit.
> 
[ ... ]
Yes, it looks quite reasonable.  Please go ahead with the full testing
cycle and consider the patch pre-approved once complete.

jeff

Re: GCSE considers read only memory clobbered by function calls.

[Paolo Bonzini]

> It appears that GCSE considers "read only memory" as call clobbered, which
> is not the case in CSE. I have took the test for read-only memory from CSE
> and add it to GCSE where we compute the transparency.

My wild guess is that this was not possible when MEM_READONLY_P was 
RTX_UNCHANGING_P, and now it is.

Paolo

Re: GCSE considers read only memory clobbered by function calls.

[Richard Henderson]

On Mon, May 09, 2005 at 05:45:24PM +0300, Mostafa Hagog wrote:
>         EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
>           {
> !             if (! MEM_READONLY_P (x))

Looks like you should push this check here:

	case MEM:
	  if (!MEM_READONLY_P (x))
	    {
	      ...
	    }
	  x = XEXP (x, 0);
	  goto repeat;



r~

Re: GCSE considers read only memory clobbered by function calls.

[Mostafa Hagog]

Richard Henderson <rth@redhat.com> wrote on 09/05/2005 19:35:34:

> On Mon, May 09, 2005 at 05:45:24PM +0300, Mostafa Hagog wrote:
> >         EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
> >           {
> > !             if (! MEM_READONLY_P (x))
>
> Looks like you should push this check here:
>
>    case MEM:
>      if (!MEM_READONLY_P (x))
>        {
>          ...
>        }
>      x = XEXP (x, 0);
>      goto repeat;
Yes, I agree, no need to waste compile time on those checks, we
know that there is no memory modifications for read-only memory.

>
>
>
> r~

Re: GCSE considers read only memory clobbered by function calls.

[Mostafa Hagog]

Paolo Bonzini <bonzini@gnu.org> wrote on 09/05/2005 18:09:10:

> > It appears that GCSE considers "read only memory" as call clobbered,
which
> > is not the case in CSE. I have took the test for read-only memory from
CSE
> > and add it to GCSE where we compute the transparency.
>
> My wild guess is that this was not possible when MEM_READONLY_P was
> RTX_UNCHANGING_P, and now it is.

What wasn't possible? the fact that GCSE considers MEM_READONLY_P call
clobbered or the fact that CSE does not consider it such that?

Mostafa.

Re: GCSE considers read only memory clobbered by function calls.

[Mostafa Hagog]

I want to add the below example as a regression test; the difference
between the success and failure is the position of a load operation. Is
there a possibility to check this using the scan-assembler?

Mostafa Hagog  wrote on 09/05/2005 17:45:24:
>
>
>
>
> It appears that GCSE considers "read only memory" as call clobbered,
which
> is not the case in CSE. I have took the test for read-only memory from
CSE
> and add it to GCSE where we compute the transparency. Here is a patch
that
> does so. This patch makes gcse eliminate redundant loads after stores for
> the following example. The difference is seen when we compile with the
> options:
> "-O3 --param max-gcse-passes=3" with/without "-fgcse-las" (on a
> powerpc-linux target). If this looks a reasonable change I will
> regression-test and bootstrap the patch and ask for commit.
>
> Example:
>
> #define CALL_FPTR(fptr) (*fptr)
> #define MY_FOO_CHECK() if (my_foo_var) my_foo_func()
>
> int my_foo_var;
>
> struct my_foo_struct {
>     int my_dummy_field;
>     int *(*ppaddr)(int);
> };
>
> struct my_foo_struct *my_foo_record;
>
> int my_main_foo(int n)
> {
>     while ((my_foo_record = CALL_FPTR(my_foo_record->ppaddr)(n))) {
>         MY_FOO_CHECK();
>     }
>
>     return 0;
> }
>
> The patch:
>
> 2005-05-09 Mostafa Hagog <mustafa@il.ibm.com>
>
>       * gcse.c (compute_transp): use MEM_READONLY_P in case of MEM.
>
> Index: gcse.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/gcse.c,v
> retrieving revision 1.340
> diff -c -p -r1.340 gcse.c
> *** gcse.c      23 Apr 2005 21:27:44 -0000      1.340
> --- gcse.c      5 May 2005 12:54:04 -0000
> *************** compute_transp (rtx x, int indx, sbitmap
> *** 2470,2479 ****
>            do any list walking for them.  */
>         EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
>           {
> !           if (set_p)
> !             SET_BIT (bmap[bb_index], indx);
> !           else
> !             RESET_BIT (bmap[bb_index], indx);
>           }
>
>         /* Now iterate over the blocks which have memory modifications
> --- 2470,2482 ----
>            do any list walking for them.  */
>         EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
>           {
> !             if (! MEM_READONLY_P (x))
> !               {
> !                 if (set_p)
> !                   SET_BIT (bmap[bb_index], indx);
> !                 else
> !                   RESET_BIT (bmap[bb_index], indx);
> !               }
>           }
>
>

Re: GCSE considers read only memory clobbered by function calls.

[Richard Henderson]

On Tue, May 10, 2005 at 11:37:50AM +0300, Mostafa Hagog wrote:
> I want to add the below example as a regression test; the difference
> between the success and failure is the position of a load operation. Is
> there a possibility to check this using the scan-assembler?

No.


r~

Re: GCSE considers read only memory clobbered by function calls.

[Mostafa Hagog]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]






Jeffrey A Law <law@redhat.com> wrote on 09/05/2005 18:17:45:

> Yes, it looks quite reasonable.  Please go ahead with the full testing
> cycle and consider the patch pre-approved once complete.
>
I have changed the patch according to some feedbacks that I have got -- the
main idea didn't change. One change is to check the MEM_READONLY_P flag
also in load_killed_in_block. The new patch is attached below. bootstrap
passed (with -O2 -g -fgcse -fgcse-las --param max-gcse-passes=3) and no new
regressions on powerpc-apple-darwin.

Since I have changed the patch a bit I am asking again for approval to
commit.

2005-05-23 Mostafa Hagog <mustafa@il.ibm.com>

      * gcse.c (compute_transp, load_killed_in_block): use MEM_READONLY_P.

(See attached file: gcse_las3.patch)

[-- Attachment #2: gcse_las3.patch --]
[-- Type: application/octet-stream, Size: 3478 bytes --]

Index: gcse.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/gcse.c,v
retrieving revision 1.340
diff -c -p -r1.340 gcse.c
*** gcse.c	23 Apr 2005 21:27:44 -0000	1.340
--- gcse.c	22 May 2005 07:10:19 -0000
*************** static int
*** 1370,1375 ****
--- 1370,1380 ----
  load_killed_in_block_p (basic_block bb, int uid_limit, rtx x, int avail_p)
  {
    rtx list_entry = modify_mem_list[bb->index];
+ 
+   /* If this is a readonly then we aren't going to be chaning it.  */
+   if (MEM_READONLY_P (x))
+     return 0;
+ 
    while (list_entry)
      {
        rtx setter;
*************** compute_transp (rtx x, int indx, sbitmap
*** 2462,2512 ****
        return;
  
      case MEM:
!       {
! 	bitmap_iterator bi;
! 	unsigned bb_index;
  
! 	/* First handle all the blocks with calls.  We don't need to
! 	   do any list walking for them.  */
! 	EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
! 	  {
! 	    if (set_p)
! 	      SET_BIT (bmap[bb_index], indx);
! 	    else
! 	      RESET_BIT (bmap[bb_index], indx);
! 	  }
  
! 	/* Now iterate over the blocks which have memory modifications
! 	   but which do not have any calls.  */
! 	EXECUTE_IF_AND_COMPL_IN_BITMAP (modify_mem_list_set, blocks_with_calls,
! 					0, bb_index, bi)
! 	  {
! 	    rtx list_entry = canon_modify_mem_list[bb_index];
  
! 	    while (list_entry)
  	      {
! 		rtx dest, dest_addr;
  
! 		/* LIST_ENTRY must be an INSN of some kind that sets memory.
! 		   Examine each hunk of memory that is modified.  */
  
! 		dest = XEXP (list_entry, 0);
! 		list_entry = XEXP (list_entry, 1);
! 		dest_addr = XEXP (list_entry, 0);
  
! 		if (canon_true_dependence (dest, GET_MODE (dest), dest_addr,
! 					   x, rtx_addr_varies_p))
! 		  {
! 		    if (set_p)
! 		      SET_BIT (bmap[bb_index], indx);
! 		    else
! 		      RESET_BIT (bmap[bb_index], indx);
! 		    break;
! 		  }
! 		list_entry = XEXP (list_entry, 1);
  	      }
! 	  }
!       }
  
        x = XEXP (x, 0);
        goto repeat;
--- 2467,2520 ----
        return;
  
      case MEM:
!       if (! MEM_READONLY_P (x))
! 	{
  
! 	  bitmap_iterator bi;
! 	  unsigned bb_index;
  
! 	  /* First handle all the blocks with calls.  We don't need to
! 	     do any list walking for them.  */
! 	  EXECUTE_IF_SET_IN_BITMAP (blocks_with_calls, 0, bb_index, bi)
! 	    {
! 	      if (set_p)
! 		SET_BIT (bmap[bb_index], indx);
! 	      else
! 		RESET_BIT (bmap[bb_index], indx);
! 	    }
  
! 	    /* Now iterate over the blocks which have memory modifications
! 	       but which do not have any calls.  */
! 	    EXECUTE_IF_AND_COMPL_IN_BITMAP (modify_mem_list_set, 
! 					    blocks_with_calls,
! 					    0, bb_index, bi)
  	      {
! 		rtx list_entry = canon_modify_mem_list[bb_index];
  
! 		while (list_entry)
! 		  {
! 		    rtx dest, dest_addr;
  
! 		    /* LIST_ENTRY must be an INSN of some kind that sets memory.
! 		       Examine each hunk of memory that is modified.  */
  
! 		    dest = XEXP (list_entry, 0);
! 		    list_entry = XEXP (list_entry, 1);
! 		    dest_addr = XEXP (list_entry, 0);
! 
! 		    if (canon_true_dependence (dest, GET_MODE (dest), dest_addr,
! 					       x, rtx_addr_varies_p))
! 		      {
! 			if (set_p)
! 			  SET_BIT (bmap[bb_index], indx);
! 			else
! 			  RESET_BIT (bmap[bb_index], indx);
! 			break;
! 		      }
! 		    list_entry = XEXP (list_entry, 1);
! 	          }
  	      }
! 	}
  
        x = XEXP (x, 0);
        goto repeat;

Re: GCSE considers read only memory clobbered by function calls.

[Jeffrey A Law]

On Mon, 2005-05-23 at 10:47 +0300, Mostafa Hagog wrote:
> 
> 
> 
> 
> Jeffrey A Law <law@redhat.com> wrote on 09/05/2005 18:17:45:
> 
> > Yes, it looks quite reasonable.  Please go ahead with the full testing
> > cycle and consider the patch pre-approved once complete.
> >
> I have changed the patch according to some feedbacks that I have got -- the
> main idea didn't change. One change is to check the MEM_READONLY_P flag
> also in load_killed_in_block. The new patch is attached below. bootstrap
> passed (with -O2 -g -fgcse -fgcse-las --param max-gcse-passes=3) and no new
> regressions on powerpc-apple-darwin.
> 
> Since I have changed the patch a bit I am asking again for approval to
> commit.
> 
> 2005-05-23 Mostafa Hagog <mustafa@il.ibm.com>
> 
>       * gcse.c (compute_transp, load_killed_in_block): use MEM_READONLY_P.
This is fine.  Please install.

Thanks,
Jeff