Add support for acquire, release, and acq_rel memory ordering in PNaCl

Add support for acquire, release, and acq_rel memory ordering in PNaCl

PNaCl currently upgrades all atomic operations to seq_cst.

As discussed in:
  https://groups.google.com/forum/#!topic/native-client-dev/wh1jEr9nsfk
PNaCl should support more than just seq_cst memory ordering to offer full access to the hardware's capabilities. For now we're still holding off on comsume ordering (which no compiler implements) and relaxed ordering (which is highly desirable performance-wise but has unsolved theoretical issues with out-of-thin-air values).

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4029
R=stichnot@chromium.org
TEST= make check

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=9dd1dea7a54652afee2582c8d5686a16feb406fa

[JF, 2015-01-07 01:13:40]

I'm not super satisfied with the testing yet, but I think that the bulk of the
code review can start and I'll update the patch with better testing.

[Jim Stichnoth, 2015-01-07 21:21:01]

Will you be adding tests of architecture-specific lowering for these new
options?

https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:171: llvm_unreachable("Invalid
memory order");
Should these llvm_unreachable calls be stronger, i.e. report_fatal_error ?

https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
File test/Transforms/NaCl/atomic/atomic_others.ll (left):

https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
test/Transforms/NaCl/atomic/atomic_others.ll:5: ; value 6). This will change
once we support other memory orderings.
Should there be a "this may change..." comment left in, or is this pretty much
the final set of consistency options we are going to support?

[JF, 2015-01-07 21:33:57]

> Will you be adding tests of architecture-specific lowering for
> these new options?

I wasn't intending to add LLVM-side tests: these instructions are supported by
LLVM and tested the same way load/store/add/multiply are tested. I do have tests
on the NaCl side for things like GCC's __sync_* builtins, so these should batch
some bad behavior.


The tests I still want to add are for cmpxchg's complexities.

https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):

https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:171: llvm_unreachable("Invalid
memory order");
On 2015/01/07 21:21:01, stichnot wrote:
> Should these llvm_unreachable calls be stronger, i.e. report_fatal_error ?

This should truly be unreachable given the code above:
 I.ule(NaCl::MemoryOrderInvalid) || I.uge(NaCl::MemoryOrderNum)

I'd use report_fatal_error in cases that aren't obviously unreachable.

https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
File test/Transforms/NaCl/atomic/atomic_others.ll (left):

https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
test/Transforms/NaCl/atomic/atomic_others.ll:5: ; value 6). This will change
once we support other memory orderings.
On 2015/01/07 21:21:01, stichnot wrote:
> Should there be a "this may change..." comment left in, or is this pretty much
> the final set of consistency options we are going to support?

I'm not sure when/how it'll change: consume will likely be renamed to something
else (in which case it's a new memory ordering) and the verdict is still out on
relaxed. I'd rather not explain the subtleties here.

This CL's change was exactly as intended when the comment was written, but the
future changes are pretty unknown.

[Jim Stichnoth, 2015-01-07 23:26:59]

On 2015/01/07 21:33:57, JF wrote:
> > Will you be adding tests of architecture-specific lowering for
> > these new options?
> 
> I wasn't intending to add LLVM-side tests: these instructions are supported by
> LLVM and tested the same way load/store/add/multiply are tested. I do have
tests
> on the NaCl side for things like GCC's __sync_* builtins, so these should
batch
> some bad behavior.
> 
> 
> The tests I still want to add are for cmpxchg's complexities.
> 
>
https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
> File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right):
> 
>
https://codereview.chromium.org/791053006/diff/1/lib/Analysis/NaCl/PNaClABIVe...
> lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:171: llvm_unreachable("Invalid
> memory order");
> On 2015/01/07 21:21:01, stichnot wrote:
> > Should these llvm_unreachable calls be stronger, i.e. report_fatal_error ?
> 
> This should truly be unreachable given the code above:
>  I.ule(NaCl::MemoryOrderInvalid) || I.uge(NaCl::MemoryOrderNum)
> 
> I'd use report_fatal_error in cases that aren't obviously unreachable.
> 
>
https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
> File test/Transforms/NaCl/atomic/atomic_others.ll (left):
> 
>
https://codereview.chromium.org/791053006/diff/1/test/Transforms/NaCl/atomic/...
> test/Transforms/NaCl/atomic/atomic_others.ll:5: ; value 6). This will change
> once we support other memory orderings.
> On 2015/01/07 21:21:01, stichnot wrote:
> > Should there be a "this may change..." comment left in, or is this pretty
much
> > the final set of consistency options we are going to support?
> 
> I'm not sure when/how it'll change: consume will likely be renamed to
something
> else (in which case it's a new memory ordering) and the verdict is still out
on
> relaxed. I'd rather not explain the subtleties here.
> 
> This CL's change was exactly as intended when the comment was written, but the
> future changes are pretty unknown.

That all sounds good.  I guess I'll wait to see the cmpxchg tests.

[JF, 2015-01-08 00:25:09]

> That all sounds good.  I guess I'll wait to see the cmpxchg tests.

I added some tests. The abi-atomics.ll test finds a bug that I need to fix. I
also want to test the other atomic functions in that file.

[JF, 2015-01-08 01:08:24]

On 2015/01/08 00:25:09, JF wrote:
> > That all sounds good.  I guess I'll wait to see the cmpxchg tests.
> 
> I added some tests. The abi-atomics.ll test finds a bug that I need to fix. I
> also want to test the other atomic functions in that file.

Bugs are now stomped.

[Jim Stichnoth, 2015-01-08 19:29:25]

otherwise lgtm

https://codereview.chromium.org/791053006/diff/180001/test/NaCl/PNaClABI/abi-...
File test/NaCl/PNaClABI/abi-atomics.ll (right):

https://codereview.chromium.org/791053006/diff/180001/test/NaCl/PNaClABI/abi-...
test/NaCl/PNaClABI/abi-atomics.ll:3: declare void
@llvm.memcpy.p0i8.p0i8.i32(i8*, i8*, i32, i32, i1)
Can these 3 be removed?  They aren't being used in this test.

(fence.all() and is.lock.free() also aren't used, and same for the non .i32
versions, but it's nice to see them as part of the complete list of atomic
intrinsics.)

https://codereview.chromium.org/791053006/diff/180001/test/Transforms/NaCl/re...
File test/Transforms/NaCl/resolve-pnacl-intrinsics.ll (right):

https://codereview.chromium.org/791053006/diff/180001/test/Transforms/NaCl/re...
test/Transforms/NaCl/resolve-pnacl-intrinsics.ll:1: ; RUN: opt < %s
-resolve-pnacl-intrinsics -S | FileCheck %s -check-prefix=CLEANED
Since you're touching this file, make this 80-char?

[JF, 2015-01-08 19:36:25]

Tree is currently closed. Will commit when it's open.

https://codereview.chromium.org/791053006/diff/180001/test/NaCl/PNaClABI/abi-...
File test/NaCl/PNaClABI/abi-atomics.ll (right):

https://codereview.chromium.org/791053006/diff/180001/test/NaCl/PNaClABI/abi-...
test/NaCl/PNaClABI/abi-atomics.ll:3: declare void
@llvm.memcpy.p0i8.p0i8.i32(i8*, i8*, i32, i32, i1)
On 2015/01/08 19:29:25, stichnot wrote:
> Can these 3 be removed?  They aren't being used in this test.
> 
> (fence.all() and is.lock.free() also aren't used, and same for the non .i32
> versions, but it's nice to see them as part of the complete list of atomic
> intrinsics.)

Done.

https://codereview.chromium.org/791053006/diff/180001/test/Transforms/NaCl/re...
File test/Transforms/NaCl/resolve-pnacl-intrinsics.ll (right):

https://codereview.chromium.org/791053006/diff/180001/test/Transforms/NaCl/re...
test/Transforms/NaCl/resolve-pnacl-intrinsics.ll:1: ; RUN: opt < %s
-resolve-pnacl-intrinsics -S | FileCheck %s -check-prefix=CLEANED
On 2015/01/08 19:29:25, stichnot wrote:
> Since you're touching this file, make this 80-char?

Done.

[JF, 2015-01-08 20:20:50]

Committed patchset #12 (id:220001) manually as
9dd1dea7a54652afee2582c8d5686a16feb406fa (presubmit successful).

[JF, 2015-01-20 22:15:13]

Note that this CL was slightly changed in:
  https://codereview.chromium.org/857403002