[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

Jakub Jelinek  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 CC||jakub at gcc dot gnu.org
 Resolution|--- |FIXED

--- Comment #8 from Jakub Jelinek  ---
Author: acsawdey
Date: Thu Sep 29 16:21:20 2016
New Revision: 240625

URL: https://gcc.gnu.org/viewcvs?rev=240625=gcc=rev
Log:
2016-09-29  Bernd Schmidt  

* builtins.c (expand_builtin_memcmp): don't swap args unless
result is only being compared with zero.


Modified:
trunk/gcc/ChangeLog
trunk/gcc/builtins.c

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #7 from Jakub Jelinek  ---
Author: jakub
Date: Wed Nov  9 16:21:45 2016
New Revision: 242007

URL: https://gcc.gnu.org/viewcvs?rev=242007=gcc=rev
Log:
PR target/77718
* builtins.c (expand_builtin_memcmp): Formatting fix.

* gcc.c-torture/execute/pr77718.c: New test.

Added:
trunk/gcc/testsuite/gcc.c-torture/execute/pr77718.c
Modified:
trunk/gcc/ChangeLog
trunk/gcc/builtins.c
trunk/gcc/testsuite/ChangeLog

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #6 from acsawdey at gcc dot gnu.org ---
I can confirm that Bernd's patch fixes the issue, bootstraps on powerpc64le,
and does not cause any new regression test failures in "make check".

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[acsawdey at linux dot vnet.ibm.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #5 from Aaron Sawdey  ---
On Tue, 2016-09-27 at 14:30 +, bernds at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718
> 
> --- Comment #4 from Bernd Schmidt  ---
> So something like this maybe?
> 
> Index: builtins.c
> ===
> --- builtins.c  (revision 240511)
> +++ builtins.c  (working copy)
> @@ -3707,11 +3707,13 @@ expand_builtin_memcmp (tree exp, rtx tar
> 
>    by_pieces_constfn constfn = NULL;
> 
> -  const char *src_str = c_getstr (arg1);
> -  if (src_str == NULL)
> -src_str = c_getstr (arg2);
> -  else
> -std::swap (arg1_rtx, arg2_rtx);
> +  const char *src_str = c_getstr (arg2);
> +  if (result_eq && src_str == NULL)
> +{
> +  src_str = c_getstr (arg1);
> +  if (src_str != NULL)
> +   std::swap (arg1_rtx, arg2_rtx);
> +}
> 
>    /* If SRC is a string constant and block move would be done
>   by pieces, we can avoid loading the string from memory
Bernd,
  Yeah that is about what I was thinking it needed. I'll test this on
ppc64le.

Thanks,
    Aaron

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[bernds at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #4 from Bernd Schmidt  ---
So something like this maybe?

Index: builtins.c
===
--- builtins.c  (revision 240511)
+++ builtins.c  (working copy)
@@ -3707,11 +3707,13 @@ expand_builtin_memcmp (tree exp, rtx tar

   by_pieces_constfn constfn = NULL;

-  const char *src_str = c_getstr (arg1);
-  if (src_str == NULL)
-src_str = c_getstr (arg2);
-  else
-std::swap (arg1_rtx, arg2_rtx);
+  const char *src_str = c_getstr (arg2);
+  if (result_eq && src_str == NULL)
+{
+  src_str = c_getstr (arg1);
+  if (src_str != NULL)
+   std::swap (arg1_rtx, arg2_rtx);
+}

   /* If SRC is a string constant and block move would be done
  by pieces, we can avoid loading the string from memory

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[acsawdey at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #3 from acsawdey at gcc dot gnu.org ---
Bernd,
  I identified the problem on powerpc64le. I think it may not show up on x86_64
because of differences in how builtin memcmp gets expanded. Normally this is
hidden because if both strings are constant, this gets handled by folding. It
only shows up in the test case because the length given is longer than the
string length, which prevents folding (undefined behavior).

It seems clear to me that it is only legitimate to swap the args in the
result_eq case because otherwise you are changing the sign of the result.

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[bernds at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

--- Comment #2 from Bernd Schmidt  ---
Over here the testcase seems not to arrive in this function, and it prints the
same value (-5) twice, which I think is supposed to be expected?

Please clarify.

[Bug middle-end/77718] [7 Regression] expand_builtin_memcmp swaps args

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77718

Richard Biener  changed:

   What|Removed |Added

   Keywords||wrong-code
 Status|UNCONFIRMED |NEW
   Last reconfirmed||2016-09-26
   Target Milestone|--- |7.0
Summary|expand_builtin_memcmp swaps |[7 Regression]
   |args|expand_builtin_memcmp swaps
   ||args
 Ever confirmed|0   |1

--- Comment #1 from Richard Biener  ---
Confirmed - we may only swap if result_eq.