[RFC] PR 20331: modified_between_p always returns 0 for read-onlymemory

[Kaz Kojima <kkojima at rr dot iij4u dot or dot jp>]

Hi,

PR 20331 is a wrong-code generation problem on sh64-elf target
which is a regression from 3.3 to later versions.  I thought
that it's a genuine target problem, but I'm unsure about it
after tracing what is going on.  Let me explain this PR.
The small C code

extern __attribute__((pure)) int pf (int *);
extern int bar;

int foo (void) { return pf (&bar);}

is wrongly compiled with -fPIC on sh64-elf.  The sh64 backend
produces the rtl sequence for &bar like

(insn 9 8 10 (set (subreg:DI (reg:SI 162) 0)
  (const:DI (sign_extend:DI (truncate:HI (ashiftrt:DI (const:SI (unspec [
         (symbol_ref:SI ("bar") [flags 0x40] <var_decl 0xb7f60cb0 bar>)] 7))
     (const_int 16 [0x10])))))) -1 (nil)
  (nil))

(insn 10 9 11 (set (subreg:DI (reg:SI 162) 0)
  (ior:DI (ashift:DI (subreg:DI (reg:SI 162) 0) (const_int 16 [0x10]))
     (zero_extend:DI (truncate:HI
       (const:DI (sign_extend:DI (truncate:HI (const:SI (unspec [
	  (symbol_ref:SI ("bar") [flags 0x40] <var_decl 0xb7f60cb0 bar>)] 7))))))))) -1 (nil)
  (nil))

(insn 11 10 12 (set (reg:SI 163)
  (plus:SI (reg:SI 162) (reg:SI 12 r12))) -1 (nil)
  (nil))

(insn 12 11 13 (set (reg:SI 161)
  (mem/u:SI (reg:SI 163) [0 S4 A32])) -1 (nil)
  (expr_list:REG_EQUAL (symbol_ref:SI ("bar") [flags 0x40] <var_decl 0xb7f60cb0 bar>)
     (nil)))

(insn 13 12 14 (set (reg:SI 2 r2)
  (reg:SI 161)) -1 (nil)
  (expr_list:REG_EQUAL (symbol_ref:SI ("bar") [flags 0x40] <var_decl 0xb7f60cb0 bar>)
     (nil)))

but the following part of optabs.c:emit_libcall_block moves the insn 12
to the top of insns and the insn 9-11 are removed as unnecessary insns
at the later pass.

  /* First emit all insns that set pseudos.  Remove them from the list as
     we go.  Avoid insns that set pseudos which were referenced in previous
     insns.  These can be generated by move_by_pieces, for example,
     to update an address.  Similarly, avoid insns that reference things
     set in previous insns.  */

  for (insn = insns; insn; insn = next)
    {
      rtx set = single_set (insn);
      rtx note;
...
      next = NEXT_INSN (insn);

      if (set != 0 && REG_P (SET_DEST (set))
	  && REGNO (SET_DEST (set)) >= FIRST_PSEUDO_REGISTER
	  && (insn == insns
	      || ((! INSN_P(insns)
		   || ! reg_mentioned_p (SET_DEST (set), PATTERN (insns)))
		  && ! reg_used_between_p (SET_DEST (set), insns, insn)
		  && ! modified_in_p (SET_SRC (set), insns)
		  && ! modified_between_p (SET_SRC (set), insns, insn))))
	{
	  if (PREV_INSN (insn))
	    NEXT_INSN (PREV_INSN (insn)) = next;
	  else
	    insns = next;

	  if (next)
	    PREV_INSN (next) = PREV_INSN (insn);

	  add_insn (insn);
	}

sh64 backend marks &bar as having MEM_READONLY_P attribute in PIC
case because it is a value in GOT.  Taking this attribute away,
the problem disappeared.  In that case, modified_between_p in the
above if conditional prevents the incorrect move of insn 12.
It looks that the current rtlanal.c:modified_between_p returns 0
if the rtx is MEM and has the MEM_READONLY_P attribute regardless
of its address.

int
modified_between_p (rtx x, rtx start, rtx end)
{
  enum rtx_code code = GET_CODE (x);
  const char *fmt;
  int i, j;
  rtx insn;

  if (start == end)
    return 0;

  switch (code)
    {
...
    case MEM:
      if (MEM_READONLY_P (x))
	return 0;
      if (modified_between_p (XEXP (x, 0), start, end))
	return 1;
      for (insn = NEXT_INSN (start); insn != end; insn = NEXT_INSN (insn))
	if (memory_modified_in_insn_p (x, insn))
	  return 1;
      return 0;
      break;

OTOH, I found that gcc-3.3 version of this hunk has a comment which
says the need to check the address:

    case MEM:
      /* If the memory is not constant, assume it is modified.  If it is
	constant, we still have to check the address.  */
      if (! RTX_UNCHANGING_P (x))
	return 1;
      break;

If this 3.3 comment still stands, it seems that modified_between_p
should check the address first and then MEM_READONLY_P attribute.
I've tried the appended patch on i686-pc-linux-gnu and got no errors
in bootstrap and there are no regressions in "make -k check", though
I'm not sure that it's the right thing to do.
Thoughts?

Regards,
	kaz
--
	* rtlanal.c (modified_between_p): Check its address first for
	MEM.

diff -up ORIG/gcc/gcc/rtlanal.c LOCAL/gcc/gcc/rtlanal.c
--- ORIG/gcc/gcc/rtlanal.c	2005-01-25 08:53:39.000000000 +0900
+++ LOCAL/gcc/gcc/rtlanal.c	2005-03-05 11:38:58.283303369 +0900
@@ -817,10 +817,10 @@ modified_between_p (rtx x, rtx start, rt
       return 1;
 
     case MEM:
-      if (MEM_READONLY_P (x))
-	return 0;
       if (modified_between_p (XEXP (x, 0), start, end))
 	return 1;
+      if (MEM_READONLY_P (x))
+	return 0;
       for (insn = NEXT_INSN (start); insn != end; insn = NEXT_INSN (insn))
 	if (memory_modified_in_insn_p (x, insn))
 	  return 1;