Bug 70168 - [5 Regression] Wrong code generation in __sync_val_compare_and_swap on PowerPC
Summary: [5 Regression] Wrong code generation in __sync_val_compare_and_swap on PowerPC
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 5.4.0
: P3 major
Target Milestone: 6.0
Assignee: Ulrich Weigand
URL: https://gcc.gnu.org/ml/gcc-patches/20...
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2016-03-10 18:54 UTC by Ulrich Weigand
Modified: 2016-03-14 18:19 UTC (History)
3 users (show)

See Also:
Host:
Target: powerpc64le-linux
Build:
Known to work:
Known to fail:
Last reconfirmed: 2016-03-10 00:00:00


Attachments
Patch to add retval vs. newval overlap check (267 bytes, patch)
2016-03-10 18:57 UTC, Ulrich Weigand
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ulrich Weigand 2016-03-10 18:54:05 UTC
Building the following test case on powerpc64le-linux with -O2 using the current GCC 5 branch:

unsigned long atomicAND (volatile unsigned long *memRef, unsigned long mask)
{
  unsigned long oldValue, newValue, prevValue;

  for (oldValue = *memRef; ; oldValue = prevValue)
    {
      newValue = oldValue & mask;
      if (newValue == oldValue)
        break;

      prevValue = __sync_val_compare_and_swap (memRef, oldValue, newValue);
      if (prevValue == oldValue)
        break;
    }

  return oldValue;
}

results in this assembler output:

atomicAND:
        ld 9,0(3)
        b .L6
        .p2align 4,,15
.L10:
        sync
.L3:
        ldarx 10,0,3
        cmpd 0,10,9
        bne- 0,.L4
        stdcx. 9,0,3
        bne- 0,.L3
.L4:
        isync
        cmpld 7,9,10
        beq 7,.L7
        mr 9,10
.L6:
        and 10,9,4
        cmpld 7,9,10
        bne 7,.L10
.L7:
        mr 3,10
        blr

Note how the stdcx. stores r9, which holds the original value, not the masked value.  Therefore, this will usually succeed without updating the memory location.

Debugging this problem, it turns out that rs6000_expand_atomic_compare_and_swap is called with retval (operands[1]) equal to newval (operands[4]), and the expander then proceeds to clobber newval before using it.

There is code to detect overlap of retval with oldval (operands[3]), but not with newval.  Adding the equivalent detection code fixes the problem for me.

For some reason, I can reproduce this problem only with GCC 5; the same problem should still be latently present in mainline since there's still no overlap check, but I seem unable to construct a test case that would actually cause retval == newval with mainline.
Comment 1 Ulrich Weigand 2016-03-10 18:57:02 UTC
Created attachment 37925 [details]
Patch to add retval vs. newval overlap check

This patch fixes the problem for me with the GCC 5 branch.  Not fully regression tested yet.
Comment 2 David Edelsohn 2016-03-10 19:13:04 UTC
Confirmed.
Comment 3 Ulrich Weigand 2016-03-10 23:12:50 UTC
Patch posted.
Comment 4 Ulrich Weigand 2016-03-10 23:59:16 UTC
Author: uweigand
Date: Thu Mar 10 23:58:44 2016
New Revision: 234126

URL: https://gcc.gnu.org/viewcvs?rev=234126&root=gcc&view=rev
Log:
	PR target/70168
	* config/rs6000/rs6000.c (rs6000_expand_atomic_compare_and_swap):
	Handle overlapping retval and newval.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/rs6000/rs6000.c
Comment 5 Ulrich Weigand 2016-03-10 23:59:51 UTC
Author: uweigand
Date: Thu Mar 10 23:59:20 2016
New Revision: 234127

URL: https://gcc.gnu.org/viewcvs?rev=234127&root=gcc&view=rev
Log:
	PR target/70168
	* config/rs6000/rs6000.c (rs6000_expand_atomic_compare_and_swap):
	Handle overlapping retval and newval.

Modified:
    branches/gcc-5-branch/gcc/ChangeLog
    branches/gcc-5-branch/gcc/config/rs6000/rs6000.c
Comment 6 Ulrich Weigand 2016-03-11 00:00:53 UTC
Fixed.
Comment 7 Michael Meissner 2016-03-14 18:19:17 UTC
Author: meissner
Date: Mon Mar 14 18:18:45 2016
New Revision: 234188

URL: https://gcc.gnu.org/viewcvs?rev=234188&root=gcc&view=rev
Log:
[gcc]
2016-03-14  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Backport from mainline
	2016-03-11  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/70131
	* config/rs6000/rs6000.md (round32<mode>2_fprs): Do not do the
	optimization if we have direct move.
	(roundu32<mode>2_fprs): Likewise.

2016-03-14  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	Backport from mainline
	2016-03-07  Rainer Orth  <ro@CeBiTec.Uni-Bielefeld.DE>

	PR target/62281
	* config/i386/sol2.h (STACK_REALIGN_DEFAULT): Define.

2016-03-10  Ulrich Weigand  <Ulrich.Weigand@de.ibm.com>

	PR target/70168
	* config/rs6000/rs6000.c (rs6000_expand_atomic_compare_and_swap):
	Handle overlapping retval and newval.

[gcc/testsuite]
2016-03-14  Michael Meissner  <meissner@linux.vnet.ibm.com>

	Backport from mainline
	2016-03-11  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/70131
	* gcc.target/powerpc/ppc-round2.c: New test.

2016-03-14  Dominique d'Humieres  <dominiq@lps.ens.fr>

	PR fortran/45076
	gfortran.dg/prof/prof.exp: New script.
	gfortran.dg/prof/dynamic_dispatch_6.f03: New test.


Added:
    branches/ibm/gcc-5-branch/gcc/testsuite/gcc.target/powerpc/ppc-round2.c
      - copied unchanged from r234186, branches/gcc-5-branch/gcc/testsuite/gcc.target/powerpc/ppc-round2.c
    branches/ibm/gcc-5-branch/gcc/testsuite/gfortran.dg/prof/
      - copied from r234186, branches/gcc-5-branch/gcc/testsuite/gfortran.dg/prof/
Modified:
    branches/ibm/gcc-5-branch/gcc/ChangeLog
    branches/ibm/gcc-5-branch/gcc/DATESTAMP
    branches/ibm/gcc-5-branch/gcc/config/i386/sol2.h
    branches/ibm/gcc-5-branch/gcc/config/rs6000/rs6000.c
    branches/ibm/gcc-5-branch/gcc/config/rs6000/rs6000.md
    branches/ibm/gcc-5-branch/gcc/testsuite/ChangeLog