This is an archive of the discontinued LLVM Phabricator instance.

Add __sync_fetch_and_nand (again)
ClosedPublic

Authored by hfinkel on Sep 21 2014, 10:00 AM.

Details

Reviewers
hfinkel
rsmith
Summary

Prior to GCC 4.4, __sync_fetch_and_nand was implemented as:

{ tmp = *ptr; *ptr = ~tmp & value; return tmp; }

but this was changed in GCC 4.4 to be:

{ tmp = *ptr; *ptr = ~(tmp & value); return tmp; }

in response to this change, support for sync_fetch_and_nand (and sync_nand_and_fetch) was removed in r99522 in order to avoid miscompiling code depending on the old semantics. However, at this point:

  1. Many years have passed, and the amount of code relying on the old semantics is likely smaller
  2. Through the work of many contributors, all LLVM backends have been updated such that "atomicrmw nand" provides the newer GCC 4.4+ semantics (this process was complete July of 2014 (added to the release notes in r212635).
  3. The lack of this intrinsic is now a needless impediment to porting codes from GCC to Clang (I've now seen several examples of this).

It is true, however, that we still set GNUC_MINOR to 2 (corresponding to GCC 4.2). To compensate for this, and to address the original concern regarding code relying on the old semantics, I've added a warning in this patch that specifically details the fact that the semantics have changed and that we provide the newer semantics.

Fixes PR8842.

Please review and thanks again!

Diff Detail

Event Timeline

hfinkel updated this revision to Diff 13911.Sep 21 2014, 10:00 AM
hfinkel retitled this revision from to Add __sync_fetch_and_nand (again).
hfinkel updated this object.
hfinkel edited the test plan for this revision. (Show Details)
hfinkel added a reviewer: rsmith.
hfinkel added a subscriber: Unknown Object (MLST).
rsmith edited edge metadata.Sep 26 2014, 7:25 PM

OK, enough time has passed, let's do this.

include/clang/Basic/DiagnosticSemaKinds.td
6881

I think you should pick a warning flag name that is specific to this particular change. We'd never want to put two different semantic changes in the same warning group, because we would want users to be able to turn off one of them without turning off the other.

test/CodeGen/Atomics.c
168–183

Please also check for the and instruction here.

hfinkel accepted this revision.Oct 2 2014, 1:58 PM
hfinkel added a reviewer: hfinkel.
hfinkel added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
6881

Makes sense. I'll make it sync-fetch-and-nand-semantics-changed.

test/CodeGen/Atomics.c
168–183

Done.

This revision is now accepted and ready to land.Oct 2 2014, 1:58 PM
hfinkel closed this revision.Oct 2 2014, 2:04 PM

r218905, thanks!