Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22240002:
Rework PNaCl memory ordering (Closed)
|
Created:
7 years, 4 months ago by JF Modified:
7 years, 4 months ago CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionRework PNaCl memory ordering
This CL reworks memory ordering as specified by PNaCl. The documentation needed some clarification, and the implementation needs a bit more work around volatile and __sync_synchronize to offer stronger guarantees than what LLVM intends to offer for legacy code.
There is a companion patch with Clang changes:
https://codereview.chromium.org/22294002
R=eliben@chromium.org
TEST= ninja check-all
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3475
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=77f169c
Patch Set 1 #
Total comments: 15
Patch Set 2 : Clarify documentation as suggested by eliben and jvoung. #Patch Set 3 : Reorganize documents as suggested by eliben (2). #
Total comments: 2
Patch Set 4 : Add llvm.nacl.atomic.fence.all as discussed with jvoung. #
Total comments: 2
Patch Set 5 : Plural. #Patch Set 6 : Clarify address-free as discussed with sehr. #
Messages
Total messages: 19 (0 generated)
This first draft of the CL updates the documentation to reflect the discussion that I had on the LLVM ML: https://groups.google.com/forum/#!topic/llvm-dev/dqKb9kTitbE My next step is to update the code, but I'd like a review of the documentation to make sure I'm heading in an appropriate direction.
https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst File docs/PNaClDeveloperGuide.rst (right): https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:39: are discouraged, and aren't present in bitcode. See `Volatile Memory Remove the "aren't present in bitcode" - i don't think this is relevant. The developer's guide should be read by devs who neither care about nor understand the bitcode ABI https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:47: * Inter-process communication through shared memory is limited to What does inter-process communication even mean in PNaCl? We should have a section here about it, at least as a place-holder https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:78: - Relaxed: no operation orders memory. Maybe this list does not belong here? This is user facing documentation; let's not spend too much time discussing things that are not actually supported. https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst#newcode166 docs/PNaClLangRef.rst:166: <PNaClDeveloperGuide.html>`_ for more details. It supports the usually Parse error on: "the usually C11/C++11 sequential consistency" https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst#newcode443 docs/PNaClLangRef.rst:443: These intrinsics implement the semantics of C11 ``setjmp`` and What are these reformatting changes doing here?
https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst File docs/PNaClDeveloperGuide.rst (right): https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:39: are discouraged, and aren't present in bitcode. See `Volatile Memory On 2013/08/05 18:35:54, eliben wrote: > Remove the "aren't present in bitcode" - i don't think this is relevant. The > developer's guide should be read by devs who neither care about nor understand > the bitcode ABI Yeah, this is the summary for developers. What is relevant is that volatile accesses are promoted to be sequentially consistent atomic accesses, unlike normal C/C++, not the bitcode ABI. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:74: ordering) at pexe creation time. As more C11/C++11 code allows us to Should the memory orderings change also be done by the frontend, and not at the pexe freeze point, after optimization? https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:135: after optimizations into atomic accesses with sequentially consistent re "after optimizations": volatiles get converted before optimizations. It seems like this has already been said earlier. Instead of focusing on when the conversion happens, perhaps we can say that pexes created and shipped now are created with sequential consistency, and the shipped pexes will remain that way. ... then talk about the future possibilities.
I updated the documentation with eliben's and jvoung's comments. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst File docs/PNaClDeveloperGuide.rst (right): https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:39: are discouraged, and aren't present in bitcode. See `Volatile Memory On 2013/08/05 18:35:54, eliben wrote: > Remove the "aren't present in bitcode" - i don't think this is relevant. The > developer's guide should be read by devs who neither care about nor understand > the bitcode ABI Done. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:47: * Inter-process communication through shared memory is limited to On 2013/08/05 18:35:54, eliben wrote: > What does inter-process communication even mean in PNaCl? We should have a > section here about it, at least as a place-holder Done. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:74: ordering) at pexe creation time. As more C11/C++11 code allows us to On 2013/08/05 18:59:41, jvoung (cr) wrote: > Should the memory orderings change also be done by the frontend, and not at the > pexe freeze point, after optimization? I think the current implementation should offer a good medium between caution and potential optimization. I'm relatively certain that our second release will be able to offer most memory orderings (except consume, and maybe relaxed which may have some implementation issues on weakly-ordered machines). Realistically most code today uses volatile or ``__sync_*``, which are all sequentially consistent, a PNaCl user has to do extra work to properly use other memory orderings, I think they can therefore be trusted to have tested this code fairly well. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:78: - Relaxed: no operation orders memory. On 2013/08/05 18:35:54, eliben wrote: > Maybe this list does not belong here? This is user facing documentation; let's > not spend too much time discussing things that are not actually supported. This is not an addition, I just moved it around. I think we should list these or redirect to LLVM's own developer documentation, since they are actually supported but rewritten post-opt. https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... docs/PNaClDeveloperGuide.rst:135: after optimizations into atomic accesses with sequentially consistent On 2013/08/05 18:59:41, jvoung (cr) wrote: > re "after optimizations": volatiles get converted before optimizations. > > It seems like this has already been said earlier. Instead of focusing on when > the conversion happens, perhaps we can say that pexes created and shipped now > are created with sequential consistency, and the shipped pexes will remain that > way. > > ... then talk about the future possibilities. OK, I can remove this section. https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst#newcode166 docs/PNaClLangRef.rst:166: <PNaClDeveloperGuide.html>`_ for more details. It supports the usually On 2013/08/05 18:35:54, eliben wrote: > Parse error on: "the usually C11/C++11 sequential consistency" Done. https://codereview.chromium.org/22240002/diff/1/docs/PNaClLangRef.rst#newcode443 docs/PNaClLangRef.rst:443: These intrinsics implement the semantics of C11 ``setjmp`` and On 2013/08/05 18:35:54, eliben wrote: > What are these reformatting changes doing here? Enthusiastic select-and-auto-format got a bit further than I intended.
On 2013/08/05 20:37:48, JF wrote: > I updated the documentation with eliben's and jvoung's comments. > > https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst > File docs/PNaClDeveloperGuide.rst (right): > > https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... > docs/PNaClDeveloperGuide.rst:39: are discouraged, and aren't present in bitcode. > See `Volatile Memory > On 2013/08/05 18:35:54, eliben wrote: > > Remove the "aren't present in bitcode" - i don't think this is relevant. The > > developer's guide should be read by devs who neither care about nor understand > > the bitcode ABI > > Done. > > https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... > docs/PNaClDeveloperGuide.rst:47: * Inter-process communication through shared > memory is limited to > On 2013/08/05 18:35:54, eliben wrote: > > What does inter-process communication even mean in PNaCl? We should have a > > section here about it, at least as a place-holder > > Done. If IPC is not supported, why discuss it elsewhere at all, at this time? The way I see a user-facing Developer's Guide is a potential developer reading it, wondering what PNaCl supports and how he can use it. When he reads that IPC is restricted to lock-free operations, he assumes IPC with lock-free operations is supported. Then, we say it's not supported at all. This is confusing, and does not add value. If you want to keep all of this in this document, please consider adding a "Rationale & Future Directions" appendix where you can go wild. In the main body of the document, let's not spend time describing features only to say they're not supported. > > > https://codereview.chromium.org/22240002/diff/1/docs/PNaClDeveloperGuide.rst#... > docs/PNaClDeveloperGuide.rst:78: - Relaxed: no operation orders memory. > On 2013/08/05 18:35:54, eliben wrote: > > Maybe this list does not belong here? This is user facing documentation; let's > > not spend too much time discussing things that are not actually supported. > > This is not an addition, I just moved it around. I think we should list these or > redirect to LLVM's own developer documentation, since they are actually > supported but rewritten post-opt. The doc currently says they are not supported. So are they, or are they not? Now *I'm* confused :-) See above w.r.t. basic user-facing parts vs. rationales & discussions of future capabilities. Let's keep it simple. Saying that all ordering constraints are supported but promoted to sequential consistency is simple and sufficient. Also, I notice you mention LLVM there. Should we mention LLVM at all in this document? This is intended for C++ developers writing for PNaCl. LLVM for them is an implementation detail. Can the LLVM references be replaced by C++11/C11 references?
Here is a companion patch with Clang changes: https://codereview.chromium.org/22294002 Note that this patch currently only has documentation changes, my next step is to add code changes.
See update "Reorganize documents as suggested by eliben (2)". > If IPC is not supported, why discuss it elsewhere at all, at this time? It came from the previous code review. It's a forward-looking statement that I was asked to clarify so that once the design of shared memory actually comes through the implication with atomics is clear. > The way I see a user-facing Developer's Guide is a potential developer reading > it, wondering what PNaCl supports and how he can use it. When he reads that IPC > is restricted to lock-free operations, he assumes IPC with lock-free operations > is supported. Then, we say it's not supported at all. This is confusing, and > does not add value. If you want to keep all of this in this document, please > consider adding a "Rationale & Future Directions" appendix where you can go > wild. In the main body of the document, let's not spend time describing features > only to say they're not supported. I moved things around for this, see update. > The doc currently says they are not supported. So are they, or are they not? Now > *I'm* confused :-) > See above w.r.t. basic user-facing parts vs. rationales & discussions of future > capabilities. Let's keep it simple. Saying that all ordering constraints are > supported but promoted to sequential consistency is simple and sufficient. See reorganized version. > Also, I notice you mention LLVM there. Should we mention LLVM at all in this > document? This is intended for C++ developers writing for PNaCl. LLVM for them > is an implementation detail. Can the LLVM references be replaced by C++11/C11 > references? Should be better now.
https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... File include/llvm/IR/NaClAtomicIntrinsics.h (right): https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... include/llvm/IR/NaClAtomicIntrinsics.h:110: MemoryOrderSequentiallyConsistentAll, I guess the thing with having this in the general MemoryOrder enum is that it becomes available to RMW, etc. which we don't necessarily want? That is compared to having a completely separate intrinsic w/ baked in memory order.
https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... File include/llvm/IR/NaClAtomicIntrinsics.h (right): https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... include/llvm/IR/NaClAtomicIntrinsics.h:110: MemoryOrderSequentiallyConsistentAll, On 2013/08/05 23:21:22, jvoung (cr) wrote: > I guess the thing with having this in the general MemoryOrder enum is that it > becomes available to RMW, etc. which we don't necessarily want? That is > compared to having a completely separate intrinsic w/ baked in memory order. Good point, I could make a separate @llvm.nacl.atomic.synchronization.fence intrinsic. Any preference on naming?
On 2013/08/05 23:25:50, JF wrote: > https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... > File include/llvm/IR/NaClAtomicIntrinsics.h (right): > > https://codereview.chromium.org/22240002/diff/14001/include/llvm/IR/NaClAtomi... > include/llvm/IR/NaClAtomicIntrinsics.h:110: > MemoryOrderSequentiallyConsistentAll, > On 2013/08/05 23:21:22, jvoung (cr) wrote: > > I guess the thing with having this in the general MemoryOrder enum is that it > > becomes available to RMW, etc. which we don't necessarily want? That is > > compared to having a completely separate intrinsic w/ baked in memory order. > > Good point, I could make a separate @llvm.nacl.atomic.synchronization.fence > intrinsic. Any preference on naming? The old one is llvm.nacl.atomic.fence (no "synchronization") -- how about llvm.nacl.atomic.fence_all, or something
> The old one is llvm.nacl.atomic.fence (no "synchronization") -- how about > llvm.nacl.atomic.fence_all, or something Works for me: define void @llvm.nacl.atomic.fence_all()
On 2013/08/05 23:38:07, JF wrote: > > The old one is llvm.nacl.atomic.fence (no "synchronization") -- how about > > llvm.nacl.atomic.fence_all, or something > > Works for me: > define void @llvm.nacl.atomic.fence_all() works for me
On 2013/08/06 01:02:21, jvoung (cr) wrote: > On 2013/08/05 23:38:07, JF wrote: > > > The old one is llvm.nacl.atomic.fence (no "synchronization") -- how about > > > llvm.nacl.atomic.fence_all, or something > > > > Works for me: > > define void @llvm.nacl.atomic.fence_all() > > works for me Added llvm.nacl.atomic.fence.all documentation, and removed old. Will start implementing.
I still think this document is not the right place to lay out our future plans (we can keep it in internal docs & issues, and perhaps write a blog post after release) BUT my bikeshedding budget for this CL is depleted, so LGTM (with a nit below) https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.rst File docs/PNaClDeveloperGuide.rst (right): https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.... docs/PNaClDeveloperGuide.rst:156: Future Direction "Future Directions" ?
https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.rst File docs/PNaClDeveloperGuide.rst (right): https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.... docs/PNaClDeveloperGuide.rst:156: Future Direction On 2013/08/06 16:11:35, eliben wrote: > "Future Directions" ? Done.
On 2013/08/06 16:19:33, JF wrote: > https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.rst > File docs/PNaClDeveloperGuide.rst (right): > > https://codereview.chromium.org/22240002/diff/25001/docs/PNaClDeveloperGuide.... > docs/PNaClDeveloperGuide.rst:156: Future Direction > On 2013/08/06 16:11:35, eliben wrote: > > "Future Directions" ? > > Done. LGTM.
Message was sent while issue was closed.
Committed patchset #6 manually as r77f169c (presubmit successful).
Message was sent while issue was closed.
The follow-up CL which implements what this CL documents is: https://codereview.chromium.org/22474008 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
