Re: [PATCH] Fix store motion

[Daniel Berlin <dan at cgsoftware dot com>]

Jakub Jelinek <jakub@redhat.com> writes:

> Hi!
> 
> The following testcase is miscompiled with -O3. The problem is that store
> motion handles the pure call the same way as const call and thus doesn't
> consider strcmp might ever read from buf (and moves the store to buf right
> before abort). A pure call as opposed to const call needs to be considered
> as potential store killer similarly to !CONST_OR_PURE_CALL_P() calls.
> Ok to commit?

Store motion can't possibly work anyway.
Because the test for set regs is reversed, it can only cause the
exact wrong thing to happen.
This appears to have happened because the code for store_ops_ok was
originally copied from expr_killed_p.  Of course, as the names imply,
store_ops_okay returns 1 for okay expressions, expr_killed_p returns 0
for okay expressions.  Somebody forgot to reverse the test, to take
this into account.
Store motion *should* be disabled until it is fixed (reversing the test exposes
many other bugs).
I submitted a one line patch to do this, but it was never reviewed.

> 
> 2001-10-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* gcse.c (store_killed_in_insn): Consider pure calls
> 	as potential store killers in addition to normal calls.
> 
> 	* gcc.c-torture/execute/20011024-1.c: New test.
> 
> --- gcc/gcse.c.jj	Thu Oct 11 19:59:45 2001
> +++ gcc/gcse.c	Wed Oct 24 15:05:30 2001
> @@ -6512,8 +6512,21 @@ store_killed_in_insn (x, insn)
>    
>    if (GET_CODE (insn) == CALL_INSN)
>      {
> +      /* A normal or pure call might read from pattern,
> +	 but const call might not.  */
>        if (CONST_OR_PURE_CALL_P (insn))
> -	return 0;
> +	{
> +	  rtx link;
> +
> +	  for (link = CALL_INSN_FUNCTION_USAGE (insn);
> +	       link;
> +	       link = XEXP (link, 1))
> +	    if (GET_CODE (XEXP (link, 0)) == USE
> +		&& GET_CODE (XEXP (XEXP (link, 0), 0)) == MEM
> +		&& GET_CODE (XEXP (XEXP (XEXP (link, 0), 0), 0)) == SCRATCH)
> +	      return 1;
> +	  return 0;
> +	}
>        else
>  	return 1;
>      }
> --- gcc/testsuite/gcc.c-torture/execute/20011024-1.c.jj	Wed Oct 24 15:51:19 2001
> +++ gcc/testsuite/gcc.c-torture/execute/20011024-1.c	Wed Oct 24 15:50:51 2001
> @@ -0,0 +1,22 @@
> +/* Test whether store motion recognizes pure functions as potentially reading
> +   any memory.  */
> +
> +typedef __SIZE_TYPE__ size_t;
> +extern void *memcpy (void *dest, const void *src, size_t n);
> +extern size_t strlen (const char *s);
> +extern int strcmp (const char *s1, const char *s2) __attribute__((pure));
> +
> +char buf[50];
> +
> +static void foo (void)
> +{
> +  if (memcpy (buf, "abc", 4) != buf) abort ();
> +  if (strcmp (buf, "abc")) abort ();
> +  memcpy (buf, "abcdefgh", strlen ("abcdefgh") + 1);
> +}
> +
> +int main (void)
> +{
> +  foo ();
> +  return 0;
> +}
> 
> 	Jakub

-- 
"Every so often, I like to stick my head out the window, look up,
and smile for a satellite picture.
"-Steven Wright