Re: Update on i686 bootstrap failure, please revert the patch

[Graham Stott <grahams at redhat dot com>]

All,

I think I might have a handle on why the x86 is failing with the
store motion patch installed.

It's failing to compile the special_format function correctly in gengenrtl.c
which leads to an abort when built genrtl executable is run.

Looking at the generated code its obvious what the problem is

        .type   special_format,@function
special_format:
        pushl   %ebp
        movl    %esp, %ebp
        subl    $24, %esp
        movl    %ebx, -4(%ebp)
        movl    8(%ebp), %ebx
        call    strchr
        testl   %eax, %eax
        je      .L207
        movl    $42, 4(%esp)
        movl    $1, %eax
.L211:
        movl    %ebx, (%esp)
.L206:

notice the storing of the parameters to the stack for the call
to the strchr routines have been move down passed the call :-(

The strchr prototype on my system has the "pure" attribute which
is what triggers the failure.

Here's a testcase which I think should also fail on non-x86 targets.

--------------------------------------------------------
extern void abort (void);
extern void exit  (int);

extern char * strchr (const char *, int) __attribute__((__pure__));

const char * text = "abcdef";

void
foo (string)
     const char *string;
{
   if (strchr (string, 'a') != &string[0])
     abort ();

   if (strchr (string, 'b') != &string[1])
     abort ();

   if (strchr (string, 'c') != &string[2])
     abort ();

   if (strchr (string, 'd') != &string[3])
     abort ();

   if (strchr (string, 'e') != &string[4])
     abort ();

   if (strchr (string, 'f') != &string[5])
     abort () ;

   if (strchr (string, 'z') != 0)
     abort ();

   return;
}

int main()
{
  foo (text);

  exit (0);
}
----------------------------------------------------------

The problem is I believe this piece of code in gcse.c:store_killed_in_insn

  if (GET_CODE (insn) == CALL_INSN)
    {
      if (CONST_CALL_P (insn))
        return 0;
      else
        return 1;
    }

The problem is the use of CONST_CALL_P which is a misleadingly named macro
because it really means the call is "const" or "pure", not just "const". This
is because of this piece of code in calls.c

  /* If this is a const call, then set the insn's unchanging bit.  */
  if (ecf_flags & (ECF_CONST | ECF_PURE))
    CONST_CALL_P (call_insn) = 1;

I think the use of CONST_CALL_P within gcse.c:store_killed_insn is wrong. It needs
a stronger test for just "const" call.

The only convenient way to tell a "const" form " pure" call is to also
look at the CALL_INSN_FUNCTION_USAGE for the call_insn thus:

static int
is_const_call_p (call_insn)
     rtx call_insn;
{
  rtx fusage;

  if (!CONST_CALL_P (call_insn))
    return 0; /* normal call  */

  for (fusage = CALL_INSN_FUNCTION_USAGE (call_insn);
       fusage;
       fusage = XEXP (fusage, 1))
    {
      if (GET_CODE (XEXP (fusage, 0)) == USE
          && GET_CODE (XEXP (XEXP (fusage, 0), 0)) == MEM)
        return 0; /* "pure" call */
    }

  return 1; /* "const" call  */
}

If I replace the CONST_CALL_P usage within gsce.c:store_killed_in_insn
with the new is_const_call_p routine thus

  if (GET_CODE (insn) == CALL_INSN)
    {
      if (is_const_call_p (insn))
        return 0;
      else
        return 1;
    }

I get passed the point of the original bootstrap failure.

This raises the issue how many other uses of the CONST_CALL_P
macro need to be changed to a stricter is_const_call_p test.

Anyway I think CONST_CALL_P should be renamed CONST_OR_PURE_CALL_P
and new CONST_CALL_P and PURE_CALL_P macros introduced so that one
can distinguish bewteen "const" and "pure" calls.

Graham