Hello, I discovered a failure relating to atomic NAND operation on x86 platform, which can be confirmed using the included test-case. The test was done with gcc 4.2.4, using 8-, 16-, 32- and 64-bit data width. Test cases on other logic atomic operators passes all width-size except NAND. I have read through the assembly code generated and confirm that some wrong codes are generated when no-optimization and -O. I ) Test case : /* Compile using: gcc -c -march=i686 test.c To get assembly listing: gcc -S -march=i686 test.c To get mix c and assembly listing: gcc -c -g -Wa,-a,-ad -march=i686 test.c > test-xxx.lst */ #include <stddef.h> #include <stdlib.h> #include <stdio.h> typedef unsigned char BYTE; typedef unsigned short WORD; typedef unsigned long DWORD; typedef unsigned long long QWORD; int main(void) { WORD xLoc; /* change xLoc type to BYTE, WORD, DWORD, QWORD to test different data-width */ typeof(xLoc) xIn, xOut, xExpect, i = 1; xLoc = xIn = (typeof(xIn)) ~ (1<<i); xExpect = (typeof(xIn)) ~(xIn & ((typeof(xIn)) 0x7F)); /* Both __sync_nand_and_fetch and __sync_fetch_and_nand have the same problem */ xOut = __sync_nand_and_fetch(&xLoc, (typeof(xIn)) 0x7F); if (xOut!=xExpect) printf("__sync_nand_and_fetch():; wrong result; i(%d) xIn(%x) xExpect(%x) xOut(%x) xLoc(%x)\n", i, xIn, xExpect, xOut, xLoc); return 0; } II ) Output of gcc -v -save-temps gcc -o test_bug -v -save-temps -march=i686 test_nand_bug.c Using built-in specs. Target: i686-pc-linux-gnu Configured with: ../gcc-4.2.4/configure --prefix=/opt/gcc-4.2.4 --host=i686-pc-linux-gnu --target=i686-pc-linux-gnu Thread model: posix gcc version 4.2.4 /opt/gcc-4.2.4/libexec/gcc/i686-pc-linux-gnu/4.2.4/cc1 -E -quiet -v test_nand_bug.c -march=i686 -fpch-preprocess -o test_nand_bug.i ignoring nonexistent directory "/opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4/../../../../i686-pc-linux-gnu/include" #include "..." search starts here: #include <...> search starts here: /usr/local/include /opt/gcc-4.2.4/include /opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4/include /usr/include End of search list. /opt/gcc-4.2.4/libexec/gcc/i686-pc-linux-gnu/4.2.4/cc1 -fpreprocessed test_nand_bug.i -quiet -dumpbase test_nand_bug.c -march=i686 -auxbase test_nand_bug -version -o test_nand_bug.s GNU C version 4.2.4 (i686-pc-linux-gnu) compiled by GNU C version 4.2.4. GGC heuristics: --param ggc-min-expand=47 --param ggc-min-heapsize=32052 Compiler executable checksum: 383821529167afc2e47a93836e3831a4 as -V -Qy -o test_nand_bug.o test_nand_bug.s GNU assembler version 2.13.90.0.18 (i386-redhat-linux) using BFD version 2.13.90.0.18 20030206 /opt/gcc-4.2.4/libexec/gcc/i686-pc-linux-gnu/4.2.4/collect2 --eh-frame-hdr -m elf_i386 -dynamic-linker /lib/ld-linux.so.2 -o test_bug /usr/lib/crt1.o /usr/lib/crti.o /opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4/crtbegin.o -L/opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4 -L/opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4/../../.. test_nand_bug.o -lgcc -lgcc_eh -lc -lgcc -lgcc_eh /opt/gcc-4.2.4/lib/gcc/i686-pc-linux-gnu/4.2.4/crtend.o /usr/lib/crtn.o
Confirmed.
Patch in testing.
__sync_*nand* behaves as documented. See info gcc, Atomic Builtins. or http://gcc.gnu.org/ml/gcc-patches/2005-04/msg02168.html Do you have any evidence that icc implements this differently?
Okay, now I noticed the 'nand' comment on the documentation for atomic builtins, the code does implement the 'negate and AND' logic, which is named 'nand'. On page 164 of icc "Intrinsics Reference" (http://softwarecommunity.intel.com/isn/downloads/softwareproducts/pdfs/347603.pdf), ti does not have such qualifier that 'nand' is not NAND, but negate-and-AND. I do not have any further evidence how icc actually implement __sync_*nand*. This came about when adding unit-test on atomic operation for purpose of trapping problems when porting across platform. I wonder why the overloading of 'nand' ?
(In reply to comment #4) > Okay, now I noticed the 'nand' comment on the documentation for atomic > builtins, the code does implement the 'negate and AND' logic, which is named > 'nand'. So, INVALID.
According to Intel [1]: According to Dan, __sync_fetch_and_nand intrinsic should be implemented as ~(target & val). Uros's patch is correct. [1] http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01214.html
The only problem is whether there are codes out there that depend on "NEGATE-and-AND"? Frankly speaking, when I was reminded about the 'nand' comment on gcc manual, only then I remembered many moons ago, I read that line, and also remembered saying to myself "W-T-F". However, many moons later, when I work on porting to other processor and test-the-unit-test, I relied on text-book and historical definition of the term 'NAND'. So, I think it is not an issue of what Intel ICC did, it is matter of getting the semantic of NAND the way any programmer was taught and practiced. Propagating a misnomer will only guarantee that this issue of " NAND is not NAND but 'N'AND" will come back to haunt every now-and-then. IMHO, it is better to bite the bullet now then swallow a canon later. :-)
Patch at http://gcc.gnu.org/ml/gcc-patches/2008-10/msg01224.html
Removing regression flag, as this isn't a regression, the intrinsics worked that way forever. That doesn't mean it shouldn't be changed for 4.4, for 4.3 and earlier IMHO only a warning is acceptable, not changing semantics of it.
Next revision of the patch (v3) at [1] generates a message when nand builtin is encountered. [1] http://gcc.gnu.org/ml/gcc-patches/2008-11/msg00157.html
Subject: Bug 37908 Author: uros Date: Mon Nov 17 11:19:06 2008 New Revision: 141942 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141942 Log: PR middle-end/37908 * optabs.c (expand_sync_operation): Properly handle NAND case by calculating ~(t1 & val) instead of (~t1 & val). * builtins.c (expand_builtin_sync_operation): Warn for changed semantics in NAND builtins. * c.opt (Wsync-nand): New warning option. Describe -Wsync-nand. * doc/invoke.texi (Warning options): Add Wsync-nand. * doc/extend.texi (Atomic Builtins) [__sync_fetch_and_nand]: Correct __sync_fetch_and_nand builtin operation in the example. Add a note about changed semantics in GCC 4.4. [__sync_nand_and_fetch]: Correct __sync_nand_and_fetch builtin operation in the example. Add a note about changed semantics in GCC 4.4. testsuite/ChangeLog: PR middle-end/37908 * gcc.dg/pr37908.c: New test. * gcc.dg/ia64-sync-1.c: Correct __sync_fetch_and_nand and __sync_nand_and_fetch results. Add dg-message to look for the warning about changed semantics of NAND builtin. (init_si, init_di): Change init value for __sync_fetch_and_nand to -1. (test_si, test_di): Change expected result of __sync_nand_and_fetch to ~7. * gcc.dg/ia64-sync-2.c: Correct __sync_fetch_and_nand and __sync_nand_and_fetch results. Add dg-message to look for the warning about changed semantics of NAND builtin. (init_noret_si, init_noret_di): Change init value for __sync_fetch_and_nand to -1. (init_noret_si, init_noret_di): Change expected result of __sync_nand_and_fetch to ~7. * gcc.dg/sync-2.c: Correct __sync_fetch_and_nand and __sync_nand_and_fetch results. Add dg-message to look for the warning about changed semantics of NAND builtin. (init_qi, init_qi): Change init value for __sync_fetch_and_nand to -1. (init_hi, init_hi): Change expected result of __sync_nand_and_fetch to ~7. * gcc.dg/sync-3.c: Copy from sync-2.c instead of including the c source file. * gcc.c-torture/compile/sync-1.c: Add dg-message to look for the warning about changed semantics of NAND builtin. * gcc.c-torture/compile/sync-2.c: Ditto. * gcc.c-torture/compile/sync-3.c: Ditto. Added: trunk/gcc/testsuite/gcc.dg/pr37908.c Modified: trunk/gcc/ChangeLog trunk/gcc/builtins.c trunk/gcc/c.opt trunk/gcc/doc/extend.texi trunk/gcc/doc/invoke.texi trunk/gcc/optabs.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.c-torture/compile/sync-1.c trunk/gcc/testsuite/gcc.c-torture/compile/sync-2.c trunk/gcc/testsuite/gcc.c-torture/compile/sync-3.c trunk/gcc/testsuite/gcc.dg/ia64-sync-1.c trunk/gcc/testsuite/gcc.dg/ia64-sync-2.c trunk/gcc/testsuite/gcc.dg/sync-2.c trunk/gcc/testsuite/gcc.dg/sync-3.c
Subject: Bug 37908 Author: uros Date: Fri Nov 21 07:28:27 2008 New Revision: 142082 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142082 Log: PR middle-end/37908 * config/ia64/ia64.c (ia64_expand_atomic_ope): Properly handle NAND case by calculating ~(new_reg & val) instead of (~new_reg & val). * config/ia64/sync.md (sync_nand<mode>): Change insn RTX to (not:IMODE (and:IMODE (...))). (sync_old_nand<mode>): Ditto. (sync_new_nand<mode>): Ditto. Modified: trunk/gcc/ChangeLog trunk/gcc/config/ia64/ia64.c trunk/gcc/config/ia64/sync.md trunk/gcc/testsuite/ChangeLog
This test case is failing on powerpc-apple-darwin9 as follows... Executing on host: /sw/src/fink.build/gcc44-4.3.999-20081120/darwin_objdir/gcc/xgcc -B/sw/src/fink.build/gcc44-4.3.999-20081120/darwin_objdir/gcc/ /sw/src/fink.build/gcc44-4.3.999-20081120/gcc-4.4-20081120/gcc/testsuite/gcc.dg/pr37908.c -Wsync-nand -lm -o ./pr37908.exe (timeout = 300) /sw/src/fink.build/gcc44-4.3.999-20081120/gcc-4.4-20081120/gcc/testsuite/gcc.dg/pr37908.c: In function 'main': /sw/src/fink.build/gcc44-4.3.999-20081120/gcc-4.4-20081120/gcc/testsuite/gcc.dg/pr37908.c:19: note: '__sync_nand_and_fetch' changed semantics in GCC 4.4 output is: /sw/src/fink.build/gcc44-4.3.999-20081120/gcc-4.4-20081120/gcc/testsuite/gcc.dg/pr37908.c: In function 'main': /sw/src/fink.build/gcc44-4.3.999-20081120/gcc-4.4-20081120/gcc/testsuite/gcc.dg/pr37908.c:19: note: '__sync_nand_and_fetch' changed semantics in GCC 4.4 PASS: gcc.dg/pr37908.c (test for warnings, line 19) PASS: gcc.dg/pr37908.c (test for excess errors) Setting LD_LIBRARY_PATH to :/sw/src/fink.build/gcc44-4.3.999-20081120/darwin_objdir/gcc::/sw/src/fink.build/gcc44-4.3.999-20081120/darwin_objdir/gcc:/usr/local/NMRPipe/xview/mac/lib:/usr/openwin/lib:/usr/local/NMRPipe/nmrbin.mac/lib:/usr/local/lib FAIL: gcc.dg/pr37908.c execution test
Created attachment 16742 [details] preprocessed file for /gcc.dg/pr37908.c on powerpc-apple-darwin9
Created attachment 16743 [details] assembly file for /gcc.dg/pr37908.c on powerpc-apple-darwin9
Subject: Bug 37908 Author: uros Date: Mon Dec 1 13:48:52 2008 New Revision: 142313 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142313 Log: PR middle-end/37908 * config/alpha/alpha.c (alpha_split_atomic_op): Properly handle NAND case by calculating ~(new_reg & val) instead of (~new_reg & val). * config/alpha/sync.md (sync_nand<mode>): Change insn RTX to (not:I48MODE (and:I48MODE (...))). (sync_old_nand<mode>): Ditto. (sync_new_nand<mode>): Ditto. Modified: trunk/gcc/ChangeLog trunk/gcc/config/alpha/alpha.c trunk/gcc/config/alpha/sync.md
Fixed on 4.4 branch, WONTFIX on earlier branches.