Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
17777004:
Concurrency support for PNaCl ABI (Closed)
|
Created:
7 years, 6 months ago by JF Modified:
7 years, 5 months ago Reviewers:
jvoung (off chromium), sehr (please use chromium), sehr, Derek Schuff, nfullagar1, Mark Seaborn, Jim Stichnoth, eliben CC:
native-client-reviews_googlegroups.com Base URL:
http://git.chromium.org/native_client/pnacl-llvm.git@master Visibility:
Public. |
DescriptionConcurrency support for PNaCl ABI
Add portable support for concurrency in PNaCl's ABI:
- Promote volatile to atomic.
- Promote all memory ordering to sequential consistency.
- Rewrite all atomic operations to frozen NaCl intrinsics for pexe.
- Rewrite atomic intrinsics to LLVM instructions for translation.
This change also adds documentation to the PNaCl language reference, as
well as tests where it makes sense.
A future CL could clean up more of our code which mentions atomics,
volatiles, memory orderings.
Multiple reviewers because this is a big patch:
- eliben: LLVM-fu and ResolvePNaClIntrinsics.
- dschuff: ABI stability.
- mseaborn: ABI stability.
- sehr: Tron-duty (fight for the user's programs to work).
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3475
R=dschuff@chromium.org, eliben@chromium.org, sehr@google.com
TEST= (cd ./pnacl/build/llvm_x86_64; ninja check-all) && ./pnacl/test.sh test-x86-32 && ./pnacl/test.sh test-x86-64 && ./pnacl/test.sh test-arm && ./pnacl/test.sh test-x86-32-sbtc && ./pnacl/test.sh test-x86-64-sbtc && ./pnacl/test.sh test-arm-sbtc
Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-llvm.git;a=commit;h=4c1316e
Patch Set 1 #
Total comments: 96
Patch Set 2 : Address mseaborn's comments. #Patch Set 3 : Fix whitespace. #
Total comments: 14
Patch Set 4 : Address eliben's comments. #Patch Set 5 : Address mseaborn's comments. #Patch Set 6 : Run clang-format on ResolvePNaClIntrinsics.cpp, NaClIntrinsics.h, FreezeAtomics.cpp. #Patch Set 7 : Address dschuff's comments. #Patch Set 8 : Address eliben's comments (2). #Patch Set 9 : Update documentation about IPC. #Patch Set 10 : Update PNaClLangRef to reflect the implementation work I will now go forward with. #
Total comments: 14
Patch Set 11 : Address eliben's comments (3: PNaClLangRef). #Patch Set 12 : Move most PNaClLangRef text to notes, and use more words from the C++11 standard. #Patch Set 13 : Fix some formatting. #Patch Set 14 : Move ResolvePNaClIntrinsics.cpp's init code to ctor as suggested by eliben: the resolved functions … #Patch Set 15 : Five intrinsics per operation instead of size, overloaded on size. The overload can be simplified. #Patch Set 16 : Simplify overloading and function verification. #
Total comments: 39
Patch Set 17 : Address mseaborn's comments (3). #Patch Set 18 : Address dschuff's comments (2). #
Total comments: 2
Patch Set 19 : Fix indentation. #Patch Set 20 : Rebase. #
Total comments: 8
Patch Set 21 : Clean up suggested by mseaborn. #
Total comments: 20
Patch Set 22 : Missed one cleanup file. #
Total comments: 41
Patch Set 23 : Address eliben's comments (4). #
Total comments: 1
Patch Set 24 : s/NaClIntrinsics/NaClAtomicIntrinsics/g #Patch Set 25 : Address jvoung's comments. #Patch Set 26 : Massage comments. #Patch Set 27 : Add \p to comments. #Patch Set 28 : Check byte alignment, not bit alignment: the later can overflow. Also, FunctionPass' virtual method… #Patch Set 29 : Handle volatile float and doubles. Also make sure value names are preserved. #Patch Set 30 : Clarify documentation. #
Total comments: 11
Patch Set 31 : Add notes on lock-free and signal handling, as well as address-free. #Patch Set 32 : Address eliben's comments (5). #
Total comments: 4
Patch Set 33 : Address eliben's comments (6). #Patch Set 34 : Properly handle more atomic types that are 8/16/32/64 bits. #Patch Set 35 : Fix bad merge. #Messages
Total messages: 57 (0 generated)
https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode109 docs/PNaClLangRef.rst:109: We recommend that C11/C++11 atomics be used instead of ``volatile``. Saying "we recommend" is kind of vague. If this document is a reference, it should be about what guarantees PNaCl does and doesn't provide. In W3C specs, recommendations usually go in sections labeled "non-normative". Maybe you could do something similar here, and separate the recommendations from the specification of the behaviour? https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared "Cross-program" is vague. Do you mean cross-process? I think it will be confusing to developers to say here that PNaCl doesn't support shared memory when PPAPI will add this soon. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode170 docs/PNaClLangRef.rst:170: ordering) at pexe creation time. We may relax these rules an honor the "an" -> "and" https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: I'm not keen on having this be overloaded so that it encodes multiple operations, with different arities, into a single intrinsic. Having unused arguments seems untidy. I think there should be separate intrinsics for atomicrmw, cmpxchg and fence. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], You shouldn't need one definition per int size here. Look at how the *_with_overflow intrinsics work in this file. LLVM allows defining polymorphic intrinsics. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: I don't think we have validator support for 16-bit atomics on all architectures, so we should omit this for the first release. We don't have any users of 16-bit atomics in the PNaCl toolchain libraries. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:209: case Intrinsic::nacl_atomic_64: I think we should omit 64-bit atomics, because MIPS does not support them. We don't have any users of 64-bit atomics in the PNaCl toolchain libraries. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:31: class FreezeAtomics : public ModulePass { Can you add a comment saying why this is a ModulePass? i.e. Because it adds intrinsic declarations. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:52: AtomicVisitor& operator=(const AtomicVisitor&); Use LLVM spacing style, " &" https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:56: Function* atomicIntrinsic(const Instruction &I, unsigned AtomicBitSize); Use LLVM spacing style, " *" https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:89: { Put '{' on previous line https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/PNaClABIS... File lib/Transforms/NaCl/PNaClABISimplify.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/PNaClABIS... lib/Transforms/NaCl/PNaClABISimplify.cpp:85: // Strip atomics replaces instructions with intrinsic calls. "Strip atomics" - is this referring to an old name for the pass? Maybe this should just say "Replace atomic instructions with intrinsic calls". https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ReplacePt... File lib/Transforms/NaCl/ReplacePtrsWithInts.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ReplacePt... lib/Transforms/NaCl/ReplacePtrsWithInts.cpp:502: // These atomics only operate on integer pointers, not Please undo this part of the change -- it's just a reordering https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:111: Value *Ptr(Call->getArgOperand(1)); Just write "Ptr = Call->getArgOperand(1)"? Using Var(Init) is odd for a plain old variable. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:183: if (hasUses) I think you can do the RAUW unconditionally https://codereview.chromium.org/17777004/diff/1/test/NaCl/PNaClABI/instructio... File test/NaCl/PNaClABI/instructions.ll (right): https://codereview.chromium.org/17777004/diff/1/test/NaCl/PNaClABI/instructio... test/NaCl/PNaClABI/instructions.ll:78: ; CHECK: disallowed: atomic: {{.*}} load atomic For readability, can you put all these atomic cases into a separate function? https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/replace-... File test/Transforms/NaCl/replace-ptrs-with-ints.ll (left): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/replace-... test/Transforms/NaCl/replace-ptrs-with-ints.ll:248: define i8 @load_attrs(i8* %ptr) { Please leave this in. ReplacePtrsWithInts still handles these cases OK, and it might as well stay tested. https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/resolve-... File test/Transforms/NaCl/resolve-pnacl-intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/resolve-... test/Transforms/NaCl/resolve-pnacl-intrinsics.ll:38: ; CHECK-NOT: @llvm.nacl.atomic This would be better done with the following once, at file level: ; RUN: opt < %s -resolve-pnacl-intrinsics -S | FileCheck %s -check-prefix=CLEANED ; CLEANED-NOT: @llvm.nacl.atomic The check is stricter that way.
PTAL https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode109 docs/PNaClLangRef.rst:109: We recommend that C11/C++11 atomics be used instead of ``volatile``. On 2013/06/26 14:33:41, Mark Seaborn wrote: > Saying "we recommend" is kind of vague. If this document is a reference, it > should be about what guarantees PNaCl does and doesn't provide. In W3C specs, > recommendations usually go in sections labeled "non-normative". Maybe you could > do something similar here, and separate the recommendations from the > specification of the behaviour? Should I just offer a rationale? The original LLVM Bitcode Reference manual does this. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared On 2013/06/26 14:33:41, Mark Seaborn wrote: > "Cross-program" is vague. Do you mean cross-process? I think it will be > confusing to developers to say here that PNaCl doesn't support shared memory > when PPAPI will add this soon. Yes, cross-process. Are you saying that I should avoid mentioning either? https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode170 docs/PNaClLangRef.rst:170: ordering) at pexe creation time. We may relax these rules an honor the On 2013/06/26 14:33:41, Mark Seaborn wrote: > "an" -> "and" Done. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: On 2013/06/26 14:33:41, Mark Seaborn wrote: > I'm not keen on having this be overloaded so that it encodes multiple > operations, with different arities, into a single intrinsic. Having unused > arguments seems untidy. > > I think there should be separate intrinsics for atomicrmw, cmpxchg and fence. We had this discussion last week, before I started coding anything, and I said exactly what you are advocating now. You convinced me to do what I've implemented instead: specialize per-bitwidth, but not per number of arguments. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], On 2013/06/26 14:33:41, Mark Seaborn wrote: > You shouldn't need one definition per int size here. Look at how the > *_with_overflow intrinsics work in this file. LLVM allows defining polymorphic > intrinsics. We kind of do: iAny applies to i1 through i128 as well as v2i1 through v16i64. We only want 4 of these 34 integer types. I could add a new ValueType, but that's a bigger local modification than I think we want to support. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: On 2013/06/26 14:33:41, Mark Seaborn wrote: > I don't think we have validator support for 16-bit atomics on all architectures, > so we should omit this for the first release. We don't have any users of 16-bit > atomics in the PNaCl toolchain libraries. Vlad thinks it can be in for M30. Presumably this would fail at validation-time instead of compile-time (which the SDK should do), which is still acceptable if unused, but at least it gives us the option to emulate, which we don't have otherwise. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:209: case Intrinsic::nacl_atomic_64: On 2013/06/26 14:33:41, Mark Seaborn wrote: > I think we should omit 64-bit atomics, because MIPS does not support them. We > don't have any users of 64-bit atomics in the PNaCl toolchain libraries. Our users will, and atomic support through locks is explicitly called out in the C11/C++11 standards. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:31: class FreezeAtomics : public ModulePass { On 2013/06/26 14:33:41, Mark Seaborn wrote: > Can you add a comment saying why this is a ModulePass? i.e. Because it adds > intrinsic declarations. Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:52: AtomicVisitor& operator=(const AtomicVisitor&); On 2013/06/26 14:33:41, Mark Seaborn wrote: > Use LLVM spacing style, " &" Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:56: Function* atomicIntrinsic(const Instruction &I, unsigned AtomicBitSize); On 2013/06/26 14:33:41, Mark Seaborn wrote: > Use LLVM spacing style, " *" Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:89: { On 2013/06/26 14:33:41, Mark Seaborn wrote: > Put '{' on previous line Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/PNaClABIS... File lib/Transforms/NaCl/PNaClABISimplify.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/PNaClABIS... lib/Transforms/NaCl/PNaClABISimplify.cpp:85: // Strip atomics replaces instructions with intrinsic calls. On 2013/06/26 14:33:41, Mark Seaborn wrote: > "Strip atomics" - is this referring to an old name for the pass? Maybe this > should just say "Replace atomic instructions with intrinsic calls". Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ReplacePt... File lib/Transforms/NaCl/ReplacePtrsWithInts.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ReplacePt... lib/Transforms/NaCl/ReplacePtrsWithInts.cpp:502: // These atomics only operate on integer pointers, not On 2013/06/26 14:33:41, Mark Seaborn wrote: > Please undo this part of the change -- it's just a reordering Not quite: the top comment now applies to the atomic instructions as well, which it did not before. There is indeed no code change, but the comment was incorrect. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:111: Value *Ptr(Call->getArgOperand(1)); On 2013/06/26 14:33:41, Mark Seaborn wrote: > Just write "Ptr = Call->getArgOperand(1)"? Using Var(Init) is odd for a plain > old variable. Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:183: if (hasUses) On 2013/06/26 14:33:41, Mark Seaborn wrote: > I think you can do the RAUW unconditionally No: assert(New->getType() == getType() && "replaceAllUses of value with new value of different type!"); https://codereview.chromium.org/17777004/diff/1/test/NaCl/PNaClABI/instructio... File test/NaCl/PNaClABI/instructions.ll (right): https://codereview.chromium.org/17777004/diff/1/test/NaCl/PNaClABI/instructio... test/NaCl/PNaClABI/instructions.ll:78: ; CHECK: disallowed: atomic: {{.*}} load atomic On 2013/06/26 14:33:41, Mark Seaborn wrote: > For readability, can you put all these atomic cases into a separate function? Done. https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/replace-... File test/Transforms/NaCl/replace-ptrs-with-ints.ll (left): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/replace-... test/Transforms/NaCl/replace-ptrs-with-ints.ll:248: define i8 @load_attrs(i8* %ptr) { On 2013/06/26 14:33:41, Mark Seaborn wrote: > Please leave this in. ReplacePtrsWithInts still handles these cases OK, and it > might as well stay tested. Done. https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/resolve-... File test/Transforms/NaCl/resolve-pnacl-intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/resolve-... test/Transforms/NaCl/resolve-pnacl-intrinsics.ll:38: ; CHECK-NOT: @llvm.nacl.atomic On 2013/06/26 14:33:41, Mark Seaborn wrote: > This would be better done with the following once, at file level: > > ; RUN: opt < %s -resolve-pnacl-intrinsics -S | FileCheck %s > -check-prefix=CLEANED > ; CLEANED-NOT: @llvm.nacl.atomic > > The check is stricter that way. Done.
https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode109 docs/PNaClLangRef.rst:109: We recommend that C11/C++11 atomics be used instead of ``volatile``. This document serves a very specific role: it documents the bitcode - currently in semantic terms, but later more strictly - describing the binary encoding as well. It's not user-facing documentation. JF - the sections you have added seem to be useful to programmers porting to PNaCl, and not only people interested in targeting PNaCl in other toolchains. I suppose we can leave it here for now, for lack of a better place, but eventually we should have a separate "user manual" where descriptions like these belong. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared On 2013/06/26 14:33:41, Mark Seaborn wrote: > "Cross-program" is vague. Do you mean cross-process? I think it will be > confusing to developers to say here that PNaCl doesn't support shared memory > when PPAPI will add this soon. Isn't the more commonly known term "inter-process communication" (IPC) - at least this is how it's usually called in linux https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode394 docs/PNaClLangRef.rst:394: * ``llvm.nacl.atomic.8`` It's not clear what the prototype of these intrinsics is. Since they're NaCl specific, we should document it. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h File include/llvm/IR/NaCl.h (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode1 include/llvm/IR/NaCl.h:1: //===-- llvm/IR/NaCl.h - NaCl Intrinsic Function Handling -------*- C++ -*-===// If this is just intrinsics, then perhaps NaclIntrinsics.h? The header-name NaCl.h is menacingly all-inclusive. NaClIntrinsics.h also nicely mirrors Intrinsics.h https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode11 include/llvm/IR/NaCl.h:11: // functions that are specific to NaCl. The order of these enums should Not sure what "the order of these enums" means ? https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode36 include/llvm/IR/NaCl.h:36: enum { Why not a const size_t ? https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:201: if (ReturnType != Type::getInt8Ty(C)) return false; Let's separate type checking from an intrinsic being allowed. If this intrinsic is used incorrectly by passing a 32-bit thingie into the 8-bit intrinsic, the error now will be: "Function XXX is a disallowed LLVM intrinsics" Which is sure to cause head scratching. These intrinsics are allowed. They are whitelisted. But we want to type-check them. The first line of defense is Intrinsics.td which allows to define argument types and will reject wrong ones. If code still reaches here, we should pop a different error message and die (report_fatal_error). https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:221: return false; Ditto. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:224: for (Value::const_use_iterator Call(F->use_begin()), CallEnd(F->use_end()); Is there a reason here to use constructor syntax for Call/CallEnd? It's not the LLVM style https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:227: assert(C->getNumArgOperands() == 5 && "call should have as many " Should this not be caught by LLVM's normal verification / type checking? https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:233: if (!Operation || !MemoryOrder) This is even worse :) You're type checking calls to intrinsics, and if some call is invalid you will just say the intrinsic is not allowed. but why is the call invalid? IMHO calls should be verified together with other instructions in PNaClABIVerifyFunctions, not here. And they should have descriptive error messages. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:256: // Keep 3 categories of intrinsics for now. I don't think the distinction between "always allowed" and "with restrictions" adds clarity. The "with restrictions" intrinsics *are* allowed, but we just do some further type checking on them. So the 3 on this line should stay really 3, IMHO :) https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:1: //===- FreezeAtomics.cpp - Stabilize instructions used for concurrency ----===// We didn't use "freeze" so far for this purpose. We have "expand" or "rewrite" usually, and "freeze" is actually used for the pnacl-freeze tool which is something completely different. IMHO "expand" would be just fine here. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:58: Instruction &I, const Type *T, unsigned Size, NaCl::AtomicOperation O, Please document the arguments, since there are so many https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:72: errs() << "Unhandled: " << I << '\n'; Why a separate errs()? Wouldn't it be better to fold into a single line? A useful class for combining strings to report_fatal_error is Twine https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:127: Type *IntType(Type::getIntNTy(C, S)); use = instead of constructor-syntax to conform to LLVM style. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:130: errs() << "Unhandled: " << I << '\n'; As above for combining into a single report_fatal_error. Also consider having a utility function that bails out, so you can have a single line instead of 2 in many places https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:136: for (size_t Intr = 0; Intr != NaCl::NumAtomicIntrinsics; ++Intr) Does it have to be linear search here? https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:160: // %res = load {atomic|volatile} T* %ptr ordering, align sizeof(T) Make it clearer in the comment that the first is converted to the second https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:63: if (IntrinsicID == Intrinsic::nacl_setjmp || No no no no no :-) resolveSimpleCall's comment is no longer correct Neither is its name You need to do special casing for setjmp/longjmp in two places The same function behaves quite differently for different calls Well, it's only 4 "no"s but please don't do this. Find a different way to encapsulate the common code of walking the uses. You can have a common utility method that does this and have both resolveSimpleCall and resolveAtomicCall use it for slightly different putposes. https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/atomics.ll File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/atomics.... test/Transforms/NaCl/atomics.ll:272: %1 = load volatile i16* %ptr, align 2 At least in a couple of cases, have a fuller CHECK coverage for the whole function. For example, to make sure the original load was removed.
https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode109 docs/PNaClLangRef.rst:109: We recommend that C11/C++11 atomics be used instead of ``volatile``. On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > Saying "we recommend" is kind of vague. If this document is a reference, it > > should be about what guarantees PNaCl does and doesn't provide. In W3C specs, > > recommendations usually go in sections labeled "non-normative". Maybe you > could > > do something similar here, and separate the recommendations from the > > specification of the behaviour? > > Should I just offer a rationale? The original LLVM Bitcode Reference > manual does this. Maybe. A rationale could be useful. But a rationale is still different from recommendations. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > "Cross-program" is vague. Do you mean cross-process? I think it will be > > confusing to developers to say here that PNaCl doesn't support shared memory > > when PPAPI will add this soon. > > Yes, cross-process. Are you saying that I should avoid mentioning either? Yes. Cross-process communication includes message-passing, which we do support. Since we'll soon support shared memory, I don't think you should rule it out here. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > I'm not keen on having this be overloaded so that it encodes multiple > > operations, with different arities, into a single intrinsic. Having unused > > arguments seems untidy. > > > > I think there should be separate intrinsics for atomicrmw, cmpxchg and fence. > > We had this discussion last week, before I started coding anything, and I said > exactly what you are advocating now. You convinced me to do what I've > implemented instead: specialize per-bitwidth, but not per number of arguments. I was talking about whether we should have a separate intrinsic for each "atomicrmw" opcode, i.e.: declare i32 @llvm.nacl.atomicrmw.add.i32(i32* %ptr, i32 %val) declare i32 @llvm.nacl.atomicrmw.sub.i32(i32* %ptr, i32 %val) ... or whether we have an opcode parameter for "atomicrmw", i.e. declare i32 @llvm.nacl.atomicrmw.i32(i32 %opcode, i32* %ptr, i32 %val) ; where %opcode represents "add", "sub", etc. I think the latter is slightly preferable. If you have a fast-and-dumb code generator that converts these to function calls, it's a bit easier to have one function that implements all the opcodes, and it doesn't really constrain cleverer code generators. I wasn't talking about whether "atomicrmw" and "cmpxchg" should be merged into the same intrinsic. I highly recommend writing design proposals down if you want to get good feedback! Either on an issue, or in a doc for longer proposals. Verbal discussions don't have the same precision. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > You shouldn't need one definition per int size here. Look at how the > > *_with_overflow intrinsics work in this file. LLVM allows defining > polymorphic > > intrinsics. > > We kind of do: iAny applies to i1 through i128 as well as v2i1 through v16i64. > We only want 4 of these 34 integer types. I could add a new ValueType, but > that's a bigger local modification than I think we want to support. I don't think the definition in Intrinsics.td needs to constrain which int types the atomic intrinsics can operate on, if that's what you mean. That's the job of the ABI verifier, which already limits pointer types to i8*, i16*, i32* and i64*. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > I don't think we have validator support for 16-bit atomics on all > architectures, > > so we should omit this for the first release. We don't have any users of > 16-bit > > atomics in the PNaCl toolchain libraries. > > Vlad thinks it can be in for M30. I don't think we should block PNaCl ABI stability on validator changes. > Presumably this would fail at validation-time > instead of compile-time (which the SDK should do), which is still acceptable if > unused, That's not really acceptable if it makes a pexe fail on one architecture but work on another. It breaks portability. > but at least it gives us the option to emulate, which we don't have > otherwise. If you want to emulate, you could do that as a user-toolchain-side IR pass which adds a cmpxchg loop. No need for 16-bit atomics to be in the stable ABI for now. https://codereview.chromium.org/17777004/diff/13001/test/Transforms/NaCl/repl... File test/Transforms/NaCl/replace-ptrs-with-ints.ll (left): https://codereview.chromium.org/17777004/diff/13001/test/Transforms/NaCl/repl... test/Transforms/NaCl/replace-ptrs-with-ints.ll:247: Please undo the whitespace changes in this file
https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:52: AtomicVisitor& operator=(const AtomicVisitor&); On 2013/06/26 15:52:29, JF wrote: > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > Use LLVM spacing style, " &" > > Done. Actually, why not just run this whole file through clang-format and be done with it? https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:125: change what happens at compile-time, but already-release pexes will release->released https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:159: reordered (unless a fence intervenes), separate, elided or fused separate->separated? https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:187: // T nacl.atomic.<size>(int32_t operation, T *location, T value, should be llvm.nacl.atomic.<size> when represented as an intrinsic name, right? https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:224: for (Value::const_use_iterator Call(F->use_begin()), CallEnd(F->use_end()); = instead of constructor syntax here too. https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:256: // Keep 3 categories of intrinsics for now. new! now with 33% more categories! https://codereview.chromium.org/17777004/diff/13001/lib/Transforms/NaCl/Freez... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/13001/lib/Transforms/NaCl/Freez... lib/Transforms/NaCl/FreezeAtomics.cpp:64: // some of this and has generic sanity checks. maybe also mention that T should be an atomic LoadInst or StoreInst
Uploaded a CL which address eliben's comments, PTAL. Now going through other comments. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode394 docs/PNaClLangRef.rst:394: * ``llvm.nacl.atomic.8`` On 2013/06/26 16:20:57, eliben wrote: > It's not clear what the prototype of these intrinsics is. Since they're NaCl > specific, we should document it. I moved this documentation from Intrinsics.td to this document instead. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h File include/llvm/IR/NaCl.h (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode1 include/llvm/IR/NaCl.h:1: //===-- llvm/IR/NaCl.h - NaCl Intrinsic Function Handling -------*- C++ -*-===// On 2013/06/26 16:20:57, eliben wrote: > If this is just intrinsics, then perhaps NaclIntrinsics.h? The header-name > NaCl.h is menacingly all-inclusive. > > NaClIntrinsics.h also nicely mirrors Intrinsics.h Done. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode11 include/llvm/IR/NaCl.h:11: // functions that are specific to NaCl. The order of these enums should On 2013/06/26 16:20:57, eliben wrote: > Not sure what "the order of these enums" means ? Moved to the actual enums below. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/NaCl.h#newcode36 include/llvm/IR/NaCl.h:36: enum { On 2013/06/26 16:20:57, eliben wrote: > Why not a const size_t ? Done. My C shows. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:201: if (ReturnType != Type::getInt8Ty(C)) return false; On 2013/06/26 16:20:57, eliben wrote: > Let's separate type checking from an intrinsic being allowed. If this intrinsic > is used incorrectly by passing a 32-bit thingie into the 8-bit intrinsic, the > error now will be: > > "Function XXX is a disallowed LLVM intrinsics" > > Which is sure to cause head scratching. > > These intrinsics are allowed. They are whitelisted. But we want to type-check > them. The first line of defense is Intrinsics.td which allows to define argument > types and will reject wrong ones. If code still reaches here, we should pop a > different error message and die (report_fatal_error). Done. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:221: return false; On 2013/06/26 16:20:57, eliben wrote: > Ditto. Moved to a separate function, with the above too. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:224: for (Value::const_use_iterator Call(F->use_begin()), CallEnd(F->use_end()); On 2013/06/26 16:20:57, eliben wrote: > Is there a reason here to use constructor syntax for Call/CallEnd? It's not the > LLVM style Done. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:227: assert(C->getNumArgOperands() == 5 && "call should have as many " On 2013/06/26 16:20:57, eliben wrote: > Should this not be caught by LLVM's normal verification / type checking? Yes, that's why I used an assert. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:233: if (!Operation || !MemoryOrder) On 2013/06/26 16:20:57, eliben wrote: > This is even worse :) You're type checking calls to intrinsics, and if some call > is invalid you will just say the intrinsic is not allowed. but why is the call > invalid? IMHO calls should be verified together with other instructions in > PNaClABIVerifyFunctions, not here. And they should have descriptive error > messages. I don't agree: it's part of the intrinsic's requirements that the operation and the memory order be compile-time constants. It's effectively in the signature of the function, but there's no way to have a "constexpr" type in the declaration. I added a note to that effect in the language reference, I do agree it wasn't explained. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:256: // Keep 3 categories of intrinsics for now. On 2013/06/26 16:20:57, eliben wrote: > I don't think the distinction between "always allowed" and "with restrictions" > adds clarity. The "with restrictions" intrinsics *are* allowed, but we just do > some further type checking on them. > > So the 3 on this line should stay really 3, IMHO :) If it's allowed always then it should always be allowed, i.e. ``return true``. Maybe this can be phrase better? I'll update the number for now, but I do think the comment should be technically correct, though maybe not in the proposed form. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:1: //===- FreezeAtomics.cpp - Stabilize instructions used for concurrency ----===// On 2013/06/26 16:20:57, eliben wrote: > We didn't use "freeze" so far for this purpose. We have "expand" or "rewrite" > usually, and "freeze" is actually used for the pnacl-freeze tool which is > something completely different. IMHO "expand" would be just fine here. What's expand and what's rewrite? We should distinguish pre-pexe opt transforms, and translation-time transforms. Is that what expand and rewrite are? Those are pretty bad names IMHO, when the goal here is to stabilize things, and then canonicalize them (or something like that). An all-out rename should obviously be on its own, but I honestly had a hard time navigating non-obvious file names. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:58: Instruction &I, const Type *T, unsigned Size, NaCl::AtomicOperation O, On 2013/06/26 16:20:57, eliben wrote: > Please document the arguments, since there are so many Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:72: errs() << "Unhandled: " << I << '\n'; On 2013/06/26 16:20:57, eliben wrote: > Why a separate errs()? Wouldn't it be better to fold into a single line? A > useful class for combining strings to report_fatal_error is Twine Done, I added a ToTwine function and re-wrote all the errs() code. I also folded out some of these checks, and made the error messages better. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:127: Type *IntType(Type::getIntNTy(C, S)); On 2013/06/26 16:20:57, eliben wrote: > use = instead of constructor-syntax to conform to LLVM style. Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:130: errs() << "Unhandled: " << I << '\n'; On 2013/06/26 16:20:57, eliben wrote: > As above for combining into a single report_fatal_error. Also consider having a > utility function that bails out, so you can have a single line instead of 2 in > many places Done. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:136: for (size_t Intr = 0; Intr != NaCl::NumAtomicIntrinsics; ++Intr) On 2013/06/26 16:20:57, eliben wrote: > Does it have to be linear search here? For 4 entries, especially when the bound is known at compile time, I'd say yes. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:160: // %res = load {atomic|volatile} T* %ptr ordering, align sizeof(T) On 2013/06/26 16:20:57, eliben wrote: > Make it clearer in the comment that the first is converted to the second Done, here and other visitors. https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/ResolvePN... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:63: if (IntrinsicID == Intrinsic::nacl_setjmp || On 2013/06/26 16:20:57, eliben wrote: > No no no no no :-) > > resolveSimpleCall's comment is no longer correct > Neither is its name > You need to do special casing for setjmp/longjmp in two places > The same function behaves quite differently for different calls > > Well, it's only 4 "no"s but please don't do this. Find a different way to > encapsulate the common code of walking the uses. You can have a common utility > method that does this and have both resolveSimpleCall and resolveAtomicCall use > it for slightly different putposes. Done: rewrote to use one class per type of rewrite, and one function that walks calls. https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/atomics.ll File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/1/test/Transforms/NaCl/atomics.... test/Transforms/NaCl/atomics.ll:272: %1 = load volatile i16* %ptr, align 2 On 2013/06/26 16:20:57, eliben wrote: > At least in a couple of cases, have a fuller CHECK coverage for the whole > function. For example, to make sure the original load was removed. Done. All the test functions are now: ; CHECK: @func foo define foo @func(foo) { ; CHECK-NEXT: call @foo ; CHECK-NEXT: ret ret foo }
> https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:233: if (!Operation || !MemoryOrder) > On 2013/06/26 16:20:57, eliben wrote: > > This is even worse :) You're type checking calls to intrinsics, and if some > call > > is invalid you will just say the intrinsic is not allowed. but why is the call > > invalid? IMHO calls should be verified together with other instructions in > > PNaClABIVerifyFunctions, not here. And they should have descriptive error > > messages. > > I don't agree: it's part of the intrinsic's requirements that the operation and > the memory order be compile-time constants. It's effectively in the signature of > the function, but there's no way to have a "constexpr" type in the declaration. > I added a note to that effect in the language reference, I do agree it wasn't > explained. It's not that. Our ABI verification code is split to module-level and function-level. At the module level, we check things like function signatures, parameters, global vars, etc. This is the right thing to check at module level. At the function level we check instructions, etc. If there's a certain instruction or function that's invoked not according to the ABI, this invocation should be verified *at the function level*. We don't have other instructions checked at module level currently, and there's no need to break this distinction with atomics. These calls ought to be verified in PNaClABIVerifyFunctions. > > https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... > lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:256: // Keep 3 categories of > intrinsics for now. > On 2013/06/26 16:20:57, eliben wrote: > > I don't think the distinction between "always allowed" and "with restrictions" > > adds clarity. The "with restrictions" intrinsics *are* allowed, but we just do > > some further type checking on them. > > > > So the 3 on this line should stay really 3, IMHO :) > > If it's allowed always then it should always be allowed, i.e. ``return true``. > Maybe this can be phrase better? I'll update the number for now, but I do think > the comment should be technically correct, though maybe not in the proposed > form. I still think the existing division into 3 categories makes more sense and see no reason to modify it. > > https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... > File lib/Transforms/NaCl/FreezeAtomics.cpp (right): > > https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... > lib/Transforms/NaCl/FreezeAtomics.cpp:1: //===- FreezeAtomics.cpp - Stabilize > instructions used for concurrency ----===// > On 2013/06/26 16:20:57, eliben wrote: > > We didn't use "freeze" so far for this purpose. We have "expand" or "rewrite" > > usually, and "freeze" is actually used for the pnacl-freeze tool which is > > something completely different. IMHO "expand" would be just fine here. > > What's expand and what's rewrite? We should distinguish pre-pexe opt transforms, > and translation-time transforms. Is that what expand and rewrite are? Those are > pretty bad names IMHO, when the goal here is to stabilize things, and then > canonicalize them (or something like that). > > An all-out rename should obviously be on its own, but I honestly had a hard time > navigating non-obvious file names. > I agree that the names could be better, and perhaps we should discuss a more sensical naming convention - in a separate CL. At this point, adding a new mnemonic -- freeze -- which has an overloaded meaning already (Karl's bitcode freeze/thaw) only adds to the confusion. You can go with "Rewrite" for now and open a P2 issue to have nicer names. ---- I'll also take a look at the other changes. Thanks.
PTAL, addressed mseaborn's comments. Now on to dschuff's. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode109 docs/PNaClLangRef.rst:109: We recommend that C11/C++11 atomics be used instead of ``volatile``. On 2013/06/26 16:47:01, Mark Seaborn wrote: > On 2013/06/26 15:52:29, JF wrote: > > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > > Saying "we recommend" is kind of vague. If this document is a reference, it > > > should be about what guarantees PNaCl does and doesn't provide. In W3C > specs, > > > recommendations usually go in sections labeled "non-normative". Maybe you > > could > > > do something similar here, and separate the recommendations from the > > > specification of the behaviour? > > > > Should I just offer a rationale? The original LLVM Bitcode Reference > > manual does this. > > Maybe. A rationale could be useful. But a rationale is still different from > recommendations. I meant: a rationale for the recommendation: "We recommend not using volatile because volatile doesn't do what you think it does". https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared On 2013/06/26 16:47:01, Mark Seaborn wrote: > On 2013/06/26 15:52:29, JF wrote: > > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > > "Cross-program" is vague. Do you mean cross-process? I think it will be > > > confusing to developers to say here that PNaCl doesn't support shared memory > > > when PPAPI will add this soon. > > > > Yes, cross-process. Are you saying that I should avoid mentioning either? > > Yes. Cross-process communication includes message-passing, which we do support. > Since we'll soon support shared memory, I don't think you should rule it out > here. Agreed, if we're soon going to offer IPC then it shouldn't be ruled out here. My aim with this paragraph is to specify what you can do with concurrency as well as what a PNaCl backend must guarantee and care about. What do we guarantee w.r.t. IPC? I think it's the same guarantees as cross-thread? https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: > I was talking about whether we should have a separate intrinsic for each > "atomicrmw" opcode, i.e.: > > declare i32 @llvm.nacl.atomicrmw.add.i32(i32* %ptr, i32 %val) > declare i32 @llvm.nacl.atomicrmw.sub.i32(i32* %ptr, i32 %val) > ... > > or whether we have an opcode parameter for "atomicrmw", i.e. > > declare i32 @llvm.nacl.atomicrmw.i32(i32 %opcode, i32* %ptr, i32 %val) > ; where %opcode represents "add", "sub", etc. > > I think the latter is slightly preferable. If you have a fast-and-dumb code > generator that converts these to function calls, it's a bit easier to have one > function that implements all the opcodes, and it doesn't really constrain > cleverer code generators. > > I wasn't talking about whether "atomicrmw" and "cmpxchg" should be merged into > the same intrinsic. > > I highly recommend writing design proposals down if you want to get good > feedback! Either on an issue, or in a doc for longer proposals. Verbal > discussions don't have the same precision. When I confirm that we agree multiple times, and I mention multiple times that there will be exactly four intrinsics, one per size, I assume we have an understanding. We write too many things in too many places, and it's occurred more than once that I write things down only to see you ignore them. I don't like no-win situations, and this is one. Again, we had this discussion, I proposed one intrinsic per size (i8, i16, i32, i64) times one per signature (load, store, rmw, cmpxchg, fence). You said you preferred less intrinsics, and through further discussion we agreed that having just 4 would also be cleaner when LLVM evolves. I'd rather stick with how it is now: LLVM's atomics and volatiles have pre-C11/C++11 legacy, and I can see them changing at the next major bitcode ABI change. I think the current implementation is likely to remain stable through such a change. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], > I don't think the definition in Intrinsics.td needs to constrain which int types > the atomic intrinsics can operate on, if that's what you mean. > > That's the job of the ABI verifier, which already limits pointer types to i8*, > i16*, i32* and i64*. That could be done, we'd then have exactly one intrinsic. Is there a solid argument for going this way, and forego the typechecks that Intrinsics.td gives us? As I explain above we had an agreement on 4 intrinsics. Why the change? https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: > I don't think we should block PNaCl ABI stability on validator changes. Agreed, and this in no way blocks PNaCl ABI stability. > That's not really acceptable if it makes a pexe fail on one architecture but > work on another. It breaks portability. This is also true of all validation failures on one platform and not another. 16-bit atomics in no way increase the "surface area" of unacceptability, and this is a moot point anyways because the translator can emulate on x86-32. > If you want to emulate, you could do that as a user-toolchain-side IR pass which > adds a cmpxchg loop. No need for 16-bit atomics to be in the stable ABI for > now. If we emulate then that should be determined by the translator. I don't see why the validator issue wouldn't be fixed soon, and why we should permanently penalize pexes for it. https://codereview.chromium.org/17777004/diff/13001/test/Transforms/NaCl/repl... File test/Transforms/NaCl/replace-ptrs-with-ints.ll (left): https://codereview.chromium.org/17777004/diff/13001/test/Transforms/NaCl/repl... test/Transforms/NaCl/replace-ptrs-with-ints.ll:247: On 2013/06/26 16:47:02, Mark Seaborn wrote: > Please undo the whitespace changes in this file Done.
Last two CLs address dschuff's comments (first clang-format, then others). https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Transforms/NaCl/FreezeAto... lib/Transforms/NaCl/FreezeAtomics.cpp:52: AtomicVisitor& operator=(const AtomicVisitor&); On 2013/06/26 17:03:28, Derek Schuff wrote: > On 2013/06/26 15:52:29, JF wrote: > > On 2013/06/26 14:33:41, Mark Seaborn wrote: > > > Use LLVM spacing style, " &" > > > > Done. > > Actually, why not just run this whole file through clang-format and be done with > it? Done on 3 files, the other files in the CL would have unrelated lines changed. https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:125: change what happens at compile-time, but already-release pexes will On 2013/06/26 17:03:29, Derek Schuff wrote: > release->released Done. https://codereview.chromium.org/17777004/diff/13001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:159: reordered (unless a fence intervenes), separate, elided or fused On 2013/06/26 17:03:29, Derek Schuff wrote: > separate->separated? Done. https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:187: // T nacl.atomic.<size>(int32_t operation, T *location, T value, On 2013/06/26 17:03:29, Derek Schuff wrote: > should be llvm.nacl.atomic.<size> when represented as an intrinsic name, right? Yes, I fixed all of these in a previous change. https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:224: for (Value::const_use_iterator Call(F->use_begin()), CallEnd(F->use_end()); On 2013/06/26 17:03:29, Derek Schuff wrote: > = instead of constructor syntax here too. Done in a previous change. https://codereview.chromium.org/17777004/diff/13001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:256: // Keep 3 categories of intrinsics for now. On 2013/06/26 17:03:29, Derek Schuff wrote: > new! now with 33% more categories! <marquee> https://codereview.chromium.org/17777004/diff/13001/lib/Transforms/NaCl/Freez... File lib/Transforms/NaCl/FreezeAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/13001/lib/Transforms/NaCl/Freez... lib/Transforms/NaCl/FreezeAtomics.cpp:64: // some of this and has generic sanity checks. On 2013/06/26 17:03:29, Derek Schuff wrote: > maybe also mention that T should be an atomic LoadInst or StoreInst I'm not sure I understand: T needs to be an Instruction, with at least getPointerOperand and getPointerAddressSpace. I've renamed T and updated the comment to reflect this, let me know if you meant something else.
New CL addressing the following: > It's not that. Our ABI verification code is split to module-level and > function-level. At the module level, we check things like function signatures, > parameters, global vars, etc. This is the right thing to check at module level. > At the function level we check instructions, etc. If there's a certain > instruction or function that's invoked not according to the ABI, this invocation > should be verified *at the function level*. We don't have other instructions > checked at module level currently, and there's no need to break this distinction > with atomics. These calls ought to be verified in PNaClABIVerifyFunctions. OK, I made the atomic intrinsics always allowed, so I now rely on Intrinsics.td typechecks, and use the function pass to validate that the operation and memory_order parameters are constants, and have the expected values. > I still think the existing division into 3 categories makes more sense and see > no reason to modify it. As discussed, I changed back to 3, and change "always allowed" to "allowed". > I agree that the names could be better, and perhaps we should discuss a more > sensical naming convention - in a separate CL. At this point, adding a new > mnemonic -- freeze -- which has an overloaded meaning already (Karl's bitcode > freeze/thaw) only adds to the confusion. You can go with "Rewrite" for now and > open a P2 issue to have nicer names. Meh, I don't like opening issues for things like this. One more proliferation of documents. I renamed it.
https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared On 2013/06/26 22:56:36, JF wrote: > Agreed, if we're soon going to offer IPC then it shouldn't be ruled out here. > > My aim with this paragraph is to specify what you can do with concurrency as > well as what a PNaCl backend must guarantee and care about. > > What do we guarantee w.r.t. IPC? I think it's the same guarantees as > cross-thread? Yes, the guarantees with memory shared between processes should be the same as the guarantees with memory shared between threads. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: On 2013/06/26 22:56:36, JF wrote: > Again, we had this discussion, I proposed one intrinsic per size > (i8, i16, i32, i64) times one per signature (load, store, rmw, > cmpxchg, fence). You said you preferred less intrinsics, and through > further discussion we agreed that having just 4 would also be > cleaner when LLVM evolves. I slightly preferred fewer variants specifically for cmpxchg. Maybe I took "just 4" to mean "load atomic", "store atomic", "cmpxchg" and "atomicrmw"? I had been hoping to see a design doc or rough outline on https://code.google.com/p/nativeclient/issues/detail?id=3475 before an implementation. Anyway, I think the set of intrinsics should look something like this: declare i32 @llvm.nacl.load.atomic.i32(i32 %order, i32* %ptr) declare void @llvm.nacl.store.atomic.i32(i32 %order, i32* %ptr, i32 %val) declare i32 @llvm.nacl.cmpxchg.i32(i32 %order, i32* %ptr, i32 %oldval, i32 %newval) declare i32 @llvm.nacl.atomicrmw.i32(i32 %order, i32* %ptr, i32 %val, i32 %opcode) declare void @llvm.nacl.fence(i32 %order) And then the same again for i8 operations (and later i16 when the validators support it). Maybe the names should be have ".p0i32" rather than ".i32" for consistency with memcpy/memset/etc. I don't mind whether the intrinsics have a %order argument for the memory ordering or not. I don't mind if atomicrmw is split into multiple intrinsics without %opcode, i.e. declare i32 @llvm.nacl.atomicrmw.add.i32(i32 %order, i32* %ptr, i32 %val) declare i32 @llvm.nacl.atomicrmw.sub.i32(i32 %order, i32* %ptr, i32 %val) etc. -- The idea of having %opcode is only a mild preference. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], On 2013/06/26 22:56:36, JF wrote: > > I don't think the definition in Intrinsics.td needs to constrain > > which int types the atomic intrinsics can operate on, if that's > > what you mean. That's the job of the ABI verifier, which already > > limits pointer types to i8*, i16*, i32* and i64*. > > That could be done, we'd then have exactly one intrinsic. Is there a > solid argument for going this way, and forego the typechecks that > Intrinsics.td gives us? As I explain above we had an agreement on 4 > intrinsics. Why the change? I'm not sure if we're talking at cross purposes here. This is just an implementation detail and doesn't affect the PNaCl ABI. If you have a family of intrinsics that are parameterised by size, e.g. declare void @llvm.foo.i8(i8) declare void @llvm.foo.i16(i16) declare void @llvm.foo.i32(i32) then in Intrinsics.td you can either declare this with a single declaration: def int_foo : Intrinsic<[], [llvm_anyint_ty]> or multiple: def int_foo_i8 : Intrinsic<[], [llvm_i8_ty]> def int_foo_i16 : Intrinsic<[], [llvm_i16_ty]> def int_foo_i32 : Intrinsic<[], [llvm_i32_ty]> I think LLVM always uses the former for its own intrinsics. We don't necessarily have to follow this style for PNaCl, but it might make the code more concise? https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: On 2013/06/26 22:56:36, JF wrote: > > If you want to emulate, you could do that as a user-toolchain-side IR pass > > which > > adds a cmpxchg loop. No need for 16-bit atomics to be in the stable ABI for > > now. > > If we emulate then that should be determined by the translator. I don't see why > the validator issue wouldn't be fixed soon, and why we should permanently > penalize pexes for it. I'm not suggesting *permanently* penalizing pexes. 16-bit atomics can be added to the set of operations allowed by the ABI later when they're supported on all architectures. The current situation is that this program works on x86-64 and ARM, but not on x86-32: volatile short int gvar; int main() { __sync_fetch_and_add(&gvar, 1); return 0; } We need to remove the inconsistency before M30. It would be easier if the inconsistency is resolved by this change, but I suppose it could be resolved in a later change.
I updated the documentation, other changes later. https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/1/docs/PNaClLangRef.rst#newcode143 docs/PNaClLangRef.rst:143: support cross-program communication, including through shared > Yes, the guarantees with memory shared between processes should be the same as > the guarantees with memory shared between threads. Great. I updated the comment. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:518: // Note that not all arguments are meaningful for all operations: > I slightly preferred fewer variants specifically for cmpxchg. Maybe I took > "just 4" to mean "load atomic", "store atomic", "cmpxchg" and "atomicrmw"? I > had been hoping to see a design doc or rough outline on > https://code.google.com/p/nativeclient/issues/detail?id=3475 before an > implementation. No, I definitely said one per size, at multiple occasions. https://codereview.chromium.org/17777004/diff/1/include/llvm/IR/Intrinsics.td... include/llvm/IR/Intrinsics.td:533: def int_nacl_atomic_32 : Intrinsic<[llvm_i32_ty], > I'm not sure if we're talking at cross purposes here. This is just an > implementation detail and doesn't affect the PNaCl ABI. I can implement an overloaded intrinsic, and add the type restriction in PNaClABIVerifyModule.cpp. I'm merely pointing out that: - We had an agreement, this revisit is. - Your proposal isn't actually though through. - Typechecks will be needed. Intrinsics.td will do *some* type checking, but not all of it, with overloading, whereas the current non-overloaded intrinsics get *all* their type checking. I'll do the change, but I hope you understand my frustration at working through this again. It's not a question of writing things down either: when I do write things down there are still misunderstandings with you. https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/1/lib/Analysis/NaCl/PNaClABIVer... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: case Intrinsic::nacl_atomic_16: > I'm not suggesting *permanently* penalizing pexes. 16-bit atomics can be added > to the set of operations allowed by the ABI later when they're supported on all > architectures. 16-bit atomics are supported on all architectures. I fail to see your point. > The current situation is that this program works on x86-64 and ARM, but not on > x86-32: Banning 16-bit atomics from the ABI won't make the program work either, and that's what you were originally proposing. We need one of: 1- Fix the validator. 2- Rewrite the 16-bit atomics in the translator only for x86-32. 3- Rewrite the 16-bit atomics for all pexes. I think this is pretty clearly in order of desirability. Vlad thinks he can make it for M30, and I don't think implementing 2 is hard. I don't see a need to 3, ever: it permanently penalizes pexes generated this way, on all platforms. > We need to remove the inconsistency before M30. It would be easier if the > inconsistency is resolved by this change, but I suppose it could be resolved in > a later change. Agreed, and as I explain above I think we should proceed with 2 only if 1 fails.
I've uploaded a new PNaClLangRef which reflects the changes I'll go forward with, an hopefully clarifies some of the memory model. PTAL and let me know if anything is unclear, in the meantime I'll proceed with code changes.
https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:109: The C and C++ standards mandate that ``volatile`` accesses execute in The C11 and C++11 standards... https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:116: non-atomic ones) from moving past a volatile operations: they act as "past volatile operations"? https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:119: with sequential consistency memory ordering. This eases the support of sequentially consistent memory ordering? https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:138: The memory model offered by PNaCl relies the same coding guidelines as "relies on the same" ? https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:199: - An atomic memory location must always be accesses with atomic accessed https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:436: declare iN @llvm.nacl.atomic.compare_exchange( name mismatch with above https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:441: Each of these intrinsic is overloaded on the ``iN`` argument. Integral intrinsics
I updated the PNaClLangRef with eliben's latest comments. As suggested by sehr I'll now move justifications, explanations, and user documentation to callouts/sidenotes, so that the LangRef gets to the point faster, and remains a reference for what a pexe is (not why or how we get there). In the end we'll probably fork those to a separate document. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:109: The C and C++ standards mandate that ``volatile`` accesses execute in On 2013/06/27 19:32:24, eliben wrote: > The C11 and C++11 standards... These restrictions are actually all in the previous standard IIRC: they don't talk about concurrency at all. I'll still update to C11/C++11 since it does look inconsistent. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:116: non-atomic ones) from moving past a volatile operations: they act as On 2013/06/27 19:32:24, eliben wrote: > "past volatile operations"? I mean: a regular load/store can't move above or below a volatile load/store. Clarified. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:119: with sequential consistency memory ordering. This eases the support of On 2013/06/27 19:32:24, eliben wrote: > sequentially consistent memory ordering? Done. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:138: The memory model offered by PNaCl relies the same coding guidelines as On 2013/06/27 19:32:24, eliben wrote: > "relies on the same" ? Done. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:199: - An atomic memory location must always be accesses with atomic On 2013/06/27 19:32:24, eliben wrote: > accessed Done. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:436: declare iN @llvm.nacl.atomic.compare_exchange( On 2013/06/27 19:32:24, eliben wrote: > name mismatch with above Done. I went with the C11/C++11 naming and forgot one. https://codereview.chromium.org/17777004/diff/38001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:441: Each of these intrinsic is overloaded on the ``iN`` argument. Integral On 2013/06/27 19:32:24, eliben wrote: > intrinsics Done.
As suggested by sehr I moved most PNaClLangRef text to notes, and use more words from the C++11 standard.
Adding Nicholas, to take a look at the PNaClLangRef.rst document from a potential user's perspective.
I changed the code to follow last week's proposal on intrinsic signatures, and overloading on integer types. PTAL, the code should now be almost ready to commit.
https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:514: [LLVMPointerType<LLVMMatchType<0>>, llvm_i32_ty], Can you comment the argument meanings here, please? https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:521: llvm_i32_ty], Indent by 1 more space since this is inside a list https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:22: namespace NaCl { We haven't used "namespace NaCl" so far, but there is a "namespace naclbitc". Maybe this should be "namespace nacl" here, for consistency with existing lower-case namespace names? https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:33: Non, // No parameter. "Non" -> "None"? Otherwise this is obscure - it looks like you're programming in French. :-) https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:41: Intrinsic::ID ID : 16; Using a bitfield here seems to be a micro-optimisation. It might break if upstream starts using a lot of intrinsic IDs, or allocates them non-contiguously. https://codereview.chromium.org/17777004/diff/57001/include/llvm/Transforms/N... File include/llvm/Transforms/NaCl.h (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/Transforms/N... include/llvm/Transforms/NaCl.h:25: BasicBlockPass *createExpandGetElementPtrPass(); Please don't re-sort this list -- it makes the change unnecessarily large and obscures the functional change. This list was sorted by pass name before. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:376: switch (Call->getIntrinsicID()) { Fix indentation: reduce by 2 https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:389: } break; "break" should go inside the {} block. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:460: LLVMContext &C = F.getContext(); Nit: 'C' -> 'Context'. This isn't used often enough to abbreviate, and naming LLVMContexts "Context" is the more common style. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:183: for (size_t P = 0; P != I->NumParams; ++P) Start a "{" block here since the body is multi-line. However, I suspect this code isn't needed after you rebase, since types of intrinsics are checked now. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: bool PNaClABIVerifyModule::isWhitelistedIntrinsic(const NaCl::AtomicIntrinsics *AI, Line is >80 chars https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:217: case Intrinsic::memcpy: You'll need to rebase this on top of recent changes https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/PNaCl... File lib/Transforms/NaCl/PNaClABISimplify.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/PNaCl... lib/Transforms/NaCl/PNaClABISimplify.cpp:86: PM.add(createRewriteAtomicsPass()); It would be better to place this before ExpandConstantExpr. The passes from ExpandConstantExpr onwards have subtle ordering constraints, but RewriteAtomics doesn't, so can go earlier. https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... test/NaCl/PNaClABI/intrinsics.ll:28: ; CHECK-NOT: Function llvm.nacl.atomic.llvm.nacl.atomic.load.i8 is a disallowed LLVM intrinsic These just become a single "CHECK-NOT: disallowed" after you rebase. https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... test/NaCl/PNaClABI/intrinsics.ll:48: declare i64 @llvm.nacl.atomic.load.i64(i64*, i32) I still disagree with allowing 64-bit atomics. These simply don't work with MIPS in the presence of shared memory. Please can we leave 64-bit atomics out in the first version?
just some nits https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:167: and inter-process communication (shared memory), but doesn't support should probably remove the reference to shared memory here since it's not supported in pepper yet? ... actually the 2nd sentence seems to say that pnacl supports it but the embedder doesn't. I'm not sure that in this document we want to make claims about what pnacl supports independently of Chrome (e.g. shared memory). but in any case it could be a little more clear here. https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:180: a future release, but pexes generate with the current toolchain will s/generate/generated https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:511: // Note that IntrReadWriteArgMem is used it all cases to prevent s/it/in https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/Resol... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/Resol... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:70: CallResolver(const CallResolver &); use LLVM_DELETED_FUNCTION to mark these as uncallable methods
I uploaded a CL for mseaborn's comments. On to the next set of comments. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... File include/llvm/IR/Intrinsics.td (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:511: // Note that IntrReadWriteArgMem is used it all cases to prevent On 2013/07/02 22:13:17, Derek Schuff wrote: > s/it/in Done. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:514: [LLVMPointerType<LLVMMatchType<0>>, llvm_i32_ty], On 2013/07/02 19:16:02, Mark Seaborn wrote: > Can you comment the argument meanings here, please? The comment above says "These are further documented in docs/PNaClLangRef.rst." I'd rather not replicate the exact same comment in two places. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/Intrinsic... include/llvm/IR/Intrinsics.td:521: llvm_i32_ty], On 2013/07/02 19:16:02, Mark Seaborn wrote: > Indent by 1 more space since this is inside a list Done, and below. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:22: namespace NaCl { On 2013/07/02 19:16:02, Mark Seaborn wrote: > We haven't used "namespace NaCl" so far, but there is a "namespace naclbitc". > Maybe this should be "namespace nacl" here, for consistency with existing > lower-case namespace names? The majority of LLVM namespaces are capitalized, so for consistency naclbitc should be capitalized. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:33: Non, // No parameter. On 2013/07/02 19:16:02, Mark Seaborn wrote: > "Non" -> "None"? Otherwise this is obscure - it looks like you're programming > in French. :-) Renamed to NoP. https://codereview.chromium.org/17777004/diff/57001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:41: Intrinsic::ID ID : 16; On 2013/07/02 19:16:02, Mark Seaborn wrote: > Using a bitfield here seems to be a micro-optimisation. It might break if > upstream starts using a lot of intrinsic IDs, or allocates them > non-contiguously. It's 40 bytes that are used at translation time, and I think it's a decent one without shortcomings. Coming from me that's not much of a micro-optimization, in a more constrained compiler I'd have done much more with this struct. https://codereview.chromium.org/17777004/diff/57001/include/llvm/Transforms/N... File include/llvm/Transforms/NaCl.h (right): https://codereview.chromium.org/17777004/diff/57001/include/llvm/Transforms/N... include/llvm/Transforms/NaCl.h:25: BasicBlockPass *createExpandGetElementPtrPass(); On 2013/07/02 19:16:02, Mark Seaborn wrote: > Please don't re-sort this list -- it makes the change unnecessarily large and > obscures the functional change. This list was sorted by pass name before. Unfortunate that automated sorting wasn't enforced before. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:376: switch (Call->getIntrinsicID()) { On 2013/07/02 19:16:02, Mark Seaborn wrote: > Fix indentation: reduce by 2 This is inconsistent with the rest of the file. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:389: } break; On 2013/07/02 19:16:02, Mark Seaborn wrote: > "break" should go inside the {} block. There's enough code this way in the code base, and it's nowhere in the standard. I suggest using clang-format. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:460: LLVMContext &C = F.getContext(); On 2013/07/02 19:16:02, Mark Seaborn wrote: > Nit: 'C' -> 'Context'. This isn't used often enough to abbreviate, and naming > LLVMContexts "Context" is the more common style. It's pretty much split in the middle on C versus Context. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:183: for (size_t P = 0; P != I->NumParams; ++P) On 2013/07/02 19:16:02, Mark Seaborn wrote: > Start a "{" block here since the body is multi-line. However, I suspect this > code isn't needed after you rebase, since types of intrinsics are checked now. Done. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:203: bool PNaClABIVerifyModule::isWhitelistedIntrinsic(const NaCl::AtomicIntrinsics *AI, On 2013/07/02 19:16:02, Mark Seaborn wrote: > Line is >80 chars Done. https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... test/NaCl/PNaClABI/intrinsics.ll:48: declare i64 @llvm.nacl.atomic.load.i64(i64*, i32) On 2013/07/02 19:16:02, Mark Seaborn wrote: > I still disagree with allowing 64-bit atomics. These simply don't work with > MIPS in the presence of shared memory. Please can we leave 64-bit atomics out > in the first version? Without shared memory there is no issue, even on MIPS: we offer the same guarantees as those of C11/C++11. With shared memory on MIPS (which is an issue that you only brought up today AFAICT, and have not documented anywhere) there is also no issue: - A similar problem already exists with HTML5 filesystem. - Offering 64-bit atomics at least allows things to work in some settings, instead of forcing users to first rewrite their code and recompile with vanilla load/store and not express any intent at all (which would prevent us from ever solving the issue for pexes that are in the wild). - The 64-bit atomic problem is equivalent to futex support on shared memory. It is no more work to support, and has the same solution. For now we don't have shared memory. Once we do the is_lock_free primitive will be used to determine if we support atomics on shared memory or not. It will also be possible to implement this 64-bit global lock either through futex emulation, or by mapping the 64-bit global lock on all processes that share memory. I will update the PNaClLangRef to that effect. For now we don't have shared memory and we don't support MIPS, and in the future we have a clear technical solution that works. 64-bit atomics are therefore a non-issue and will be supported in the first release. I encourage you to file a bug and mention this discussion. I've discussed this with sehr and bsy and have their agreement.
I'll take another look at the code after you've rebased the change onto origin/master -- there are some conflicts to resolve, I think. https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:376: switch (Call->getIntrinsicID()) { On 2013/07/02 23:14:54, JF wrote: > On 2013/07/02 19:16:02, Mark Seaborn wrote: > > Fix indentation: reduce by 2 > > This is inconsistent with the rest of the file. This 'switch' statement doesn't line up with the comment on the previous line or the previous block. https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... test/NaCl/PNaClABI/intrinsics.ll:48: declare i64 @llvm.nacl.atomic.load.i64(i64*, i32) On 2013/07/02 23:14:54, JF wrote: > On 2013/07/02 19:16:02, Mark Seaborn wrote: > > I still disagree with allowing 64-bit atomics. These simply > > don't work with MIPS in the presence of shared memory. > > Please can we leave 64-bit atomics out in the first version? > > Without shared memory there is no issue, even on MIPS: we > offer the same guarantees as those of C11/C++11. > > With shared memory on MIPS (which is an issue that you only > brought up today AFAICT, and have not documented anywhere) > there is also no issue: > - A similar problem already exists with HTML5 filesystem. That's not the case, because the Javascript interface does not provide atomic memory operations on files. > - Offering 64-bit atomics at least allows things to work in > some settings, instead of forcing users to first rewrite their > code and recompile with vanilla load/store and not express any > intent at all (which would prevent us from ever solving the > issue for pexes that are in the wild). Having things work in some settings is the problem: programs will potentially work on x86 and ARM, but then fail on MIPS. That is the kind of problem we've been trying to avoid, because it breaks PNaCl's portability guarantee. > - The 64-bit atomic problem is equivalent to futex support on > shared memory. It is no more work to support, and has the same > solution. It's not equivalent, because NaCl futexes are defined to be per-process. There's no way to use any of NaCl's current futex interfaces such that a futex will have cross-process behaviour on a shared memory page, so there's no way that a program will accidentally depend on such cross-process behaviour. > For now we don't have shared memory. Once we do the > is_lock_free primitive will be used to determine if we support > atomics on shared memory or not. That would also break PNaCl's portability guarantee, if a program can query is_lock_free and get different answers for 64-bit on x86 and MIPS. > It will also be possible to implement this 64-bit global lock > either through futex emulation, I'm not sure what you're proposing there?
Addressed dschuff's comments, PTAL. https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:167: and inter-process communication (shared memory), but doesn't support On 2013/07/02 22:13:17, Derek Schuff wrote: > should probably remove the reference to shared memory here since it's not > supported in pepper yet? > ... actually the 2nd sentence seems to say that pnacl supports it but the > embedder doesn't. I'm not sure that in this document we want to make claims > about what pnacl supports independently of Chrome (e.g. shared memory). but in > any case it could be a little more clear here. I clarified this entire section, please review again. https://codereview.chromium.org/17777004/diff/57001/docs/PNaClLangRef.rst#new... docs/PNaClLangRef.rst:180: a future release, but pexes generate with the current toolchain will On 2013/07/02 22:13:17, Derek Schuff wrote: > s/generate/generated Done. https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/Resol... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Transforms/NaCl/Resol... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:70: CallResolver(const CallResolver &); On 2013/07/02 22:13:17, Derek Schuff wrote: > use LLVM_DELETED_FUNCTION to mark these as uncallable methods Done, in a few other places.
https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/57001/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:376: switch (Call->getIntrinsicID()) { On 2013/07/02 23:34:10, Mark Seaborn wrote: > On 2013/07/02 23:14:54, JF wrote: > > On 2013/07/02 19:16:02, Mark Seaborn wrote: > > > Fix indentation: reduce by 2 > > > > This is inconsistent with the rest of the file. > > This 'switch' statement doesn't line up with the comment on the previous line or > the previous block. Done.
https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... File test/NaCl/PNaClABI/intrinsics.ll (right): https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrin... test/NaCl/PNaClABI/intrinsics.ll:48: declare i64 @llvm.nacl.atomic.load.i64(i64*, i32) I suggest you read the updated PNaClLangRef, it should contain the information required for the ABI and users (I'll split these documentation sources later). Please let me know if that document is unclear.
We do not support cross-process shared memory today (except ironically through HTML5 files, which we have by accident and without any thought to semantics), and as far as I know, there are no plans to add it. Furthermore, we don't have several key pieces of a cross-process shared memory implementation of any sort of synchronization (mutexes are not shareable, etc.) other than message passing. When and if we implement cross-process capable futexes, we will have what is needed to do a (slow) implementation of lock-based atomicity with very little additional implementation. As for non-determinism and undefined behavior, people can write Dekker's algorithm and get non-portable code that runs perfectly fine on x86, so we already have undefined behavior simply by having threads. We are by definition going to have the possibility of code written by users that doesn't run the same everywhere unless they code to conditions that we cannot verify mechanically. I think the right answer is a statement that the behavior of any program that relies on cross-process communication through atomic operations that are not explicitly implemented as lock free is undefined. For version one, the stronger statement that any cross-process sharing of data is undefined is in order. David On Tue, Jul 2, 2013 at 4:50 PM, <jfb@chromium.org> wrote: > > https://codereview.chromium.**org/17777004/diff/57001/test/** > NaCl/PNaClABI/intrinsics.ll<https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrinsics.ll> > File test/NaCl/PNaClABI/intrinsics.**ll (right): > > https://codereview.chromium.**org/17777004/diff/57001/test/** > NaCl/PNaClABI/intrinsics.ll#**newcode48<https://codereview.chromium.org/17777004/diff/57001/test/NaCl/PNaClABI/intrinsics.ll#newcode48> > test/NaCl/PNaClABI/intrinsics.**ll:48: declare i64 > @llvm.nacl.atomic.load.i64(**i64*, i32) > I suggest you read the updated PNaClLangRef, it should contain the > information required for the ABI and users (I'll split these > documentation sources later). Please let me know if that document is > unclear. > > https://codereview.chromium.**org/17777004/<https://codereview.chromium.org/1... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
Adding stichnot and jvoung to the review: the ABI should now pretty much be in its final form, and the code has had a few reviews.
On 2 July 2013 18:05, <jfb@chromium.org> wrote: > Adding stichnot and jvoung to the review: the ABI should now pretty much > be in > its final form, and the code has had a few reviews. > Please can you merge with the current state of the pnacl-llvm branch? You'll need to fix some merge conflicts, and I suspect some simplifications to your current ABI checker patch may be possible since intrinsic types are now checked in tip-of-tree. Cheers, Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
> Please can you merge with the current state of the pnacl-llvm branch? > You'll need to fix some merge conflicts, and I suspect some simplifications > to your current ABI checker patch may be possible since intrinsic types are > now checked in tip-of-tree. Yes, but that doesn't change the majority of the code, it can therefore be reviewed now.
I did a rebase in patch set 20. This obviously makes the CL harder to review, but it should now be fully up to date. Please review.
The langref part LGTM. I'm still going through the code, but my review is optional for that part. On Tue, Jul 2, 2013 at 8:28 PM, <jfb@chromium.org> wrote: > I did a rebase in patch set 20. This obviously makes the CL harder to > review, > but it should now be fully up to date. Please review. > > https://codereview.chromium.**org/17777004/<https://codereview.chromium.org/1... > -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to native-client-reviews+unsubscribe@googlegroups.com. To post to this group, send email to native-client-reviews@googlegroups.com. Visit this group at http://groups.google.com/group/native-client-reviews. For more options, visit https://groups.google.com/groups/opt_out.
https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:43: uint8_t NumParams; You can remove this -- it's set but not read https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:47: Type *Signature[MaxAtomicIntrinsicsParameters]; This isn't read any more https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:50: Type *Parameter(size_t P) const { return Signature[P]; } This method isn't used any more https://codereview.chromium.org/17777004/diff/75002/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/75002/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:73: bool isWhitelistedIntrinsic(const NaCl::AtomicIntrinsics *AI, This isn't defined any more after rebasing
https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:43: uint8_t NumParams; On 2013/07/03 04:55:20, Mark Seaborn wrote: > You can remove this -- it's set but not read Cleaned up. https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:47: Type *Signature[MaxAtomicIntrinsicsParameters]; On 2013/07/03 04:55:20, Mark Seaborn wrote: > This isn't read any more Done. https://codereview.chromium.org/17777004/diff/75002/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:50: Type *Parameter(size_t P) const { return Signature[P]; } On 2013/07/03 04:55:20, Mark Seaborn wrote: > This method isn't used any more Done. https://codereview.chromium.org/17777004/diff/75002/lib/Analysis/NaCl/PNaClAB... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/75002/lib/Analysis/NaCl/PNaClAB... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:73: bool isWhitelistedIntrinsic(const NaCl::AtomicIntrinsics *AI, On 2013/07/03 04:55:20, Mark Seaborn wrote: > This isn't defined any more after rebasing Done, and the include up top.
https://codereview.chromium.org/17777004/diff/103001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/103001/docs/PNaClLangRef.rst#ne... docs/PNaClLangRef.rst:173: PNaCl has varying support for concurrency and parallelism: "varying support" is a bit pessimistic. How about "supports concurrency and parallelism with some restrictions" https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:43: Intrinsic::ID ID : 16; Is there a good reason to use bitfields here? Saving memory can't be it because there are only a handful of intrinsics. https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:50: M, ID, ArrayRef<Type *>(&OverloadedType, Overloaded ? 1 : 0)); Please clarify what's going on here with a comment https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:215: const char *PNaClABIVerifyFunctions::checkInstruction(LLVMContext &C, I think you should not carry the Context and AtomicIntrinsics into this method just for the sake of verifying intrinsics. Moreover, below in runOnFunction you do: NaCl::AtomicIntrinsics AI(C); Recreating it for each function the pass runs on. Why? It would be both cleaner and more efficient to have "AI" a member of this pass, initialized in the pass constructor, and checkInstruction then also doesn't need to have these passed in. https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:302: return "atomic"; Maybe "atomic load" / "volatile load", etc? Same below for store https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:207: for (Type **T = &AtomicTypes[0], **E = T + array_lengthof(AtomicTypes); The double-pointer is an overkill here. Can't you just iterate with a numeric index? https://codereview.chromium.org/17777004/diff/103001/lib/IR/NaClIntrinsics.cpp File lib/IR/NaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:28: #define INIT(P0, P1, P2, P3, P4, INTRIN) \ Comment to explain what's going on https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:44: // Interface specifying how calls should be resolved. Explain how it's used and what has to be implemented by subclasses https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:48: raw_string_ostream OS(S); I did not check too deeply, but are you sure it's safe to do this? AFAIU a Twine does not copy, only reference https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:63: template <class Instruction> Comments on all methods in this class (and others too) https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:57: class const_iterator { Please get rid of this iterator. I found it very confusing when trying to read an understand the code, and you use it only in a couple of places. Given such an iterator, how can I know if it iterates over all overloads, or just overloads for a single ID? You can use ArrayRefs to efficiently return "array views" of parts of your intrinsic table, if you need that. Or explicit indices. Such code would be much clearer to understand to someone not intimately familiar with this iterator, and moreover - no verification of the iterator's correctness would be needed.
LGTM https://codereview.chromium.org/17777004/diff/72001/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/72001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:10: // This file describes intrinsic functions that are specific to NaCl. since these files are really only for atomic intrinsics, they should probably be named as such (unless someone plans to use them for other things). Also the comment here and in NaClIntrinsics.cpp should be updated.
https://codereview.chromium.org/17777004/diff/105001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/105001/docs/PNaClLangRef.rst#ne... docs/PNaClLangRef.rst:116: PNaCl bitcode does not support volatile memory accesses. Not sure what the original doc for this looked like, but this seems like this can be mistaken for the Clang throwing an error when it sees "volatile" at the source level. Perhaps that can be clarified... Something like "Volatile memory accesses are erased from bitcode and converted to have sequentially consistent atomic semantics at a late stage". Then the note follows with the full explanation. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:14: #ifndef LLVM_IR_NACL_H ifdef guard should be NACL_INTRINSICS something instead of NACL? https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:23: namespace NaCl { perhaps use allow lower case: namespace "nacl" We have namespace "naclbitc" and all lowercase would be more consistent. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:26: static const size_t NumAtomicIntrinsicTypes = 4; Could call this "NumAtomicIntrinsicOverloadTypes", but that's pretty long =/ https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:90: AtomicIntrinsics(const AtomicIntrinsics &) LLVM_DELETED_FUNCTION; Interesting that they didn't make llvm/Support/support_macros.h's DISALLOW_CLASS_COPY_AND_ASSIGN() have LLVM_DELETED_FUNCTION... https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cpp File lib/IR/NaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:30: for (size_t CurType = 0; CurType != NumAtomicIntrinsicTypes; ++CurType) { \ Is this always looping NumAtomicIntrinsicTypes times even with atomics that have no overloads (fence)? Might be more clear if only the ones with overloads use this loop (then you don't need the giant "P0 == Int || ..."), and the ones that don't have overloads written out separately. https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:60: return begin().I + NumAtomicIntrinsics * NumAtomicIntrinsicTypes; I guess this works because even for something without overloads (fence), you create multiple entries for it? https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/PNaC... File lib/Transforms/NaCl/PNaClABISimplify.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/PNaC... lib/Transforms/NaCl/PNaClABISimplify.cpp:98: PM.add(createRewriteAtomicsPass()); This doesn't introduce constant exprs, etc. right? Otherwise, it seems safer to do this first. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:135: llvm_unreachable("unknown atomic intrinsic"); report_fatal_error() instead so that it shows up even with LLVM is not built w/ asserts? https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:176: unsigned alignmentFromPointer(const Value *Ptr) const { Could this just be done by DataLayout's getPointerABIAlignment(), or some other method? I guess that assumes our DataLayout uses natural alignment. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:163: void AtomicVisitor::checkAlignment(const Instruction &I, unsigned Alignment, nit: Perhaps name variables AlignmentBits, SizeBits or something to be clear about the units. https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... test/Transforms/NaCl/atomics.ll:17: ; Alignment is also expected to be at least natural alignment. Is there a test for the alignment checks?
https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:128: const Twine Name(""); Can this just be inlined to the new LoadInst(...) ? It's not used anywhere else, and otherwise it's not clear what it is the name of. Should it get the name from the original Call inst? https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:176: unsigned alignmentFromPointer(const Value *Ptr) const { On 2013/07/03 17:50:26, jvoung (cr) wrote: > Could this just be done by DataLayout's getPointerABIAlignment(), or some other > method? > > I guess that assumes our DataLayout uses natural alignment. Err... sorry not getPointerABIAlignment, but getABITypeAlignment of the pointed-to-type. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:93: AtomicVisitor(Module &M) nit: Might be more useful to have public API be listed first instead of last. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:132: if (AO == NaCl::MemoryOrderInvalid) Put curlies around the if's then-statement since it's more than one line?
https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:132: if (AO == NaCl::MemoryOrderInvalid) On 2013/07/03 20:14:20, jvoung (cr) wrote: > Put curlies around the if's then-statement since it's more than one line? Inspired by Steve's stories from yesterday, JF ;-) ?
Address eliben's latest comments. https://codereview.chromium.org/17777004/diff/103001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/103001/docs/PNaClLangRef.rst#ne... docs/PNaClLangRef.rst:173: PNaCl has varying support for concurrency and parallelism: On 2013/07/03 16:06:05, eliben wrote: > "varying support" is a bit pessimistic. How about "supports concurrency and > parallelism with some restrictions" Done. https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:43: Intrinsic::ID ID : 16; On 2013/07/03 16:06:05, eliben wrote: > Is there a good reason to use bitfields here? Saving memory can't be it because > there are only a handful of intrinsics. Depending on member ordering it's shaving off at least 60 bytes, all of which are accessed on the user's device. Memory pressure matters. Sure it's not a big thing, but it's easy enough to do that it's worth it. I don't mind pre-pexe time that much, but we'll have to put quite a bit of effort into translation time. https://codereview.chromium.org/17777004/diff/103001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:50: M, ID, ArrayRef<Type *>(&OverloadedType, Overloaded ? 1 : 0)); On 2013/07/03 16:06:05, eliben wrote: > Please clarify what's going on here with a comment Done. https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:215: const char *PNaClABIVerifyFunctions::checkInstruction(LLVMContext &C, On 2013/07/03 16:06:05, eliben wrote: > I think you should not carry the Context and AtomicIntrinsics into this method > just for the sake of verifying intrinsics. Moreover, below in runOnFunction you > do: > > NaCl::AtomicIntrinsics AI(C); > > Recreating it for each function the pass runs on. Why? It would be both cleaner > and more efficient to have "AI" a member of this pass, initialized in the pass > constructor, and checkInstruction then also doesn't need to have these passed > in. Done. https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:302: return "atomic"; On 2013/07/03 16:06:05, eliben wrote: > Maybe "atomic load" / "volatile load", etc? Same below for store Done. It's a bit redundant: see my corresponding change in instructions.ll (it relies on that message). ; CHECK: disallowed: atomic load: {{.*}} load atomic %a2 = load atomic i32* %ptr seq_cst, align 4 ; CHECK: disallowed: volatile load: {{.*}} load volatile %a3 = load volatile i32* %ptr, align 4 ; CHECK: disallowed: atomic store: store atomic store atomic i32 undef, i32* %ptr seq_cst, align 4 ; CHECK: disallowed: volatile store: store volatile store volatile i32 undef, i32* %ptr, align 4 https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyModule.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyModule.cpp:207: for (Type **T = &AtomicTypes[0], **E = T + array_lengthof(AtomicTypes); On 2013/07/03 16:06:05, eliben wrote: > The double-pointer is an overkill here. Can't you just iterate with a numeric > index? Done. https://codereview.chromium.org/17777004/diff/103001/lib/IR/NaClIntrinsics.cpp File lib/IR/NaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:28: #define INIT(P0, P1, P2, P3, P4, INTRIN) \ On 2013/07/03 16:06:05, eliben wrote: > Comment to explain what's going on Done. https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:44: // Interface specifying how calls should be resolved. On 2013/07/03 16:06:05, eliben wrote: > Explain how it's used and what has to be implemented by subclasses Done. I edited this comment and a few others, and 's/walk/visit/g'. https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:48: raw_string_ostream OS(S); On 2013/07/03 16:06:05, eliben wrote: > I did not check too deeply, but are you sure it's safe to do this? AFAIU a Twine > does not copy, only reference You're correct, I should have read the Twine documentation more. I changed it to return std::string. https://codereview.chromium.org/17777004/diff/103001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:63: template <class Instruction> On 2013/07/03 16:06:05, eliben wrote: > Comments on all methods in this class (and others too) Done. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:57: class const_iterator { On 2013/07/03 16:06:05, eliben wrote: > Please get rid of this iterator. I found it very confusing when trying to read > an understand the code, and you use it only in a couple of places. Given such an > iterator, how can I know if it iterates over all overloads, or just overloads > for a single ID? You can use ArrayRefs to efficiently return "array views" of > parts of your intrinsic table, if you need that. Or explicit indices. Such code > would be much clearer to understand to someone not intimately familiar with this > iterator, and moreover - no verification of the iterator's correctness would be > needed. Awesome, ArrayRef is indeed much nicer. I changed the interface to: typedef ArrayRef<AtomicIntrinsic> View; View allIntrinsicsAndOverloads() const; View overloadsFor(Intrinsic::ID ID) const; const AtomicIntrinsic *find(Intrinsic::ID ID, Type *OverloadedType) const;
https://codereview.chromium.org/17777004/diff/72001/include/llvm/IR/NaClIntri... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/72001/include/llvm/IR/NaClIntri... include/llvm/IR/NaClIntrinsics.h:10: // This file describes intrinsic functions that are specific to NaCl. On 2013/07/03 16:08:55, Derek Schuff wrote: > since these files are really only for atomic intrinsics, they should probably be > named as such (unless someone plans to use them for other things). Also the > comment here and in NaClIntrinsics.cpp should be updated. Done in a single change.
""Depending on member ordering it's shaving off at least 60 bytes, all of which are accessed on the user's device. Memory pressure matters. Sure it's not a big thing, but it's easy enough to do that it's worth it. I don't mind pre-pexe time that much, but we'll have to put quite a bit of effort into translation time."" I won't dwell on this further because it's a stylistic issue and isn't very important, but the above isn't very convincing :-) Just consider that: 1. Bitfields are usually slower to access than discrete fields, because extra bit extracting operations are required. 2. Since this is the sandboxed translator we're talking about, the bitfields will be expanded out anyways (they're inherently unportable), so you're not saving any memory. In light of these -- and considering that bitfields make for less readable and less portable code, I would just get rid of them. But again, I don't really want to keep arguing so it's your call unless other reviewers step in. https://codereview.chromium.org/17777004/diff/124001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/124001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:61: View allIntrinsicsAndOverloads() const; Add comments that explain what each of these methods do. Do that for all public (or protected interface) methods in classes/structs you define. Also, the LLVM style-guide (which we follow for within-LLVM localmods) is public comments that describe classes, methods, etc, start with /// and refer to parameters with "\p ParamName". Doxygen-y style (meh, but consistency is most important). It's documented here: http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments And thanks for removing the iterator -- this is way better.
I addressed jvoung's comments. There are a few open questions left through this, see my comments below. Nothing major, but it would be good to conclude on them. https://codereview.chromium.org/17777004/diff/105001/docs/PNaClLangRef.rst File docs/PNaClLangRef.rst (right): https://codereview.chromium.org/17777004/diff/105001/docs/PNaClLangRef.rst#ne... docs/PNaClLangRef.rst:116: PNaCl bitcode does not support volatile memory accesses. On 2013/07/03 17:50:26, jvoung (cr) wrote: > Not sure what the original doc for this looked like, but this seems like this > can be mistaken for the Clang throwing an error when it sees "volatile" at the > source level. > > Perhaps that can be clarified... Something like "Volatile memory accesses are > erased from bitcode and converted to have sequentially consistent atomic > semantics at a late stage". > > Then the note follows with the full explanation. The text originally explained more, and I moved the explanation to the note. When we fork this document into ABI+user documentation I'll make sure I keep a concise note in the ABI document. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:14: #ifndef LLVM_IR_NACL_H On 2013/07/03 17:50:26, jvoung (cr) wrote: > ifdef guard should be NACL_INTRINSICS something instead of NACL? Done in the renaming I did for dschuff's review. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:23: namespace NaCl { On 2013/07/03 17:50:26, jvoung (cr) wrote: > perhaps use allow lower case: namespace "nacl" > > We have namespace "naclbitc" and all lowercase would be more consistent. mseaborn had the same suggestion: using uppercase is actually more consistent with other LLVM namespaces. I'd rename naclbitc instead, to match the rest of the code. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:26: static const size_t NumAtomicIntrinsicTypes = 4; On 2013/07/03 17:50:26, jvoung (cr) wrote: > Could call this "NumAtomicIntrinsicOverloadTypes", but that's pretty long =/ Done. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:90: AtomicIntrinsics(const AtomicIntrinsics &) LLVM_DELETED_FUNCTION; On 2013/07/03 17:50:26, jvoung (cr) wrote: > Interesting that they didn't make llvm/Support/support_macros.h's > DISALLOW_CLASS_COPY_AND_ASSIGN() have LLVM_DELETED_FUNCTION... Seems like an oversight that should get fixed upstream. https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cpp File lib/IR/NaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:30: for (size_t CurType = 0; CurType != NumAtomicIntrinsicTypes; ++CurType) { \ On 2013/07/03 17:50:26, jvoung (cr) wrote: > Is this always looping NumAtomicIntrinsicTypes times even with atomics that have > no overloads (fence)? > > Might be more clear if only the ones with overloads use this loop (then you > don't need the giant "P0 == Int || ..."), and the ones that don't have overloads > written out separately. Yes, it's easier in the other parts of the code to treat fence as if it may have overloads. The .Overloaded initialization should all resolver to a constant value at compile-time. Also, this code is only initialization, it shouldn't be called often. I'd like to rewrite some of the bigger logic, but at this point it should pretty much come out the same, so I'd rather do it in another CL instead. https://codereview.chromium.org/17777004/diff/105001/lib/IR/NaClIntrinsics.cp... lib/IR/NaClIntrinsics.cpp:60: return begin().I + NumAtomicIntrinsics * NumAtomicIntrinsicTypes; On 2013/07/03 17:50:26, jvoung (cr) wrote: > I guess this works because even for something without overloads (fence), you > create multiple entries for it? Yes. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/PNaC... File lib/Transforms/NaCl/PNaClABISimplify.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/PNaC... lib/Transforms/NaCl/PNaClABISimplify.cpp:98: PM.add(createRewriteAtomicsPass()); On 2013/07/03 17:50:26, jvoung (cr) wrote: > This doesn't introduce constant exprs, etc. right? Otherwise, it seems safer to > do this first. It doesn't introduce any, and I'd rather rely on values coming in to the instructions I inspect being normalized. It should only change instructions to calls, not change the arguments beyond adding ConstantInt arguments. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:128: const Twine Name(""); On 2013/07/03 20:14:20, jvoung (cr) wrote: > Can this just be inlined to the new LoadInst(...) ? > > It's not used anywhere else, and otherwise it's not clear what it is the name > of. > > Should it get the name from the original Call inst? Done. I left LoadInst as "" and setName below for all instructions. I also added name setting on the other side (AtomicVisitor::replaceInstructionWithIntrinsicCall). https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:135: llvm_unreachable("unknown atomic intrinsic"); On 2013/07/03 17:50:26, jvoung (cr) wrote: > report_fatal_error() instead so that it shows up even with LLVM is not built w/ > asserts? This should actually be unreachable, since this is only called through the following code: NaCl::AtomicIntrinsics AI(F.getParent()->getContext()); NaCl::AtomicIntrinsics::View V = AI.allIntrinsicsAndOverloads(); for (NaCl::AtomicIntrinsics::View::iterator I = V.begin(), E = V.end(); I != E; ++I) { AtomicCallResolver AtomicResolver(F, I); Changed |= visitCalls(AtomicResolver); } It seems like the right thing to assert, since user code shouldn't actually reach this. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:176: unsigned alignmentFromPointer(const Value *Ptr) const { On 2013/07/03 20:14:20, jvoung (cr) wrote: > On 2013/07/03 17:50:26, jvoung (cr) wrote: > > Could this just be done by DataLayout's getPointerABIAlignment(), or some > other > > method? > > > > I guess that assumes our DataLayout uses natural alignment. > > Err... sorry not getPointerABIAlignment, but getABITypeAlignment of the > pointed-to-type. I actually want the natural alignment for the type *pointed to*, AFAICT getABITypeAlignment gives the alignment for pointer types. It does look like DataLayout.h may have some equivalent, maybe I should do: getABIIntegerTypeAlignment(BitWidth) ? That seems overly complex, and doesn't match what RewriteAtomics.cpp does, so I'd need to change that too. Opinion? Really all we want it natural alignment, which is what this does. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:93: AtomicVisitor(Module &M) On 2013/07/03 20:14:20, jvoung (cr) wrote: > nit: Might be more useful to have public API be listed first instead of last. Done. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:132: if (AO == NaCl::MemoryOrderInvalid) On 2013/07/03 20:14:20, jvoung (cr) wrote: > Put curlies around the if's then-statement since it's more than one line? Done. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:163: void AtomicVisitor::checkAlignment(const Instruction &I, unsigned Alignment, On 2013/07/03 17:50:26, jvoung (cr) wrote: > nit: Perhaps name variables AlignmentBits, SizeBits or something to be clear > about the units. Done. https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... test/Transforms/NaCl/atomics.ll:17: ; Alignment is also expected to be at least natural alignment. On 2013/07/03 17:50:26, jvoung (cr) wrote: > Is there a test for the alignment checks? How would you construct this test? It currently kills opt when a misaligned load/store is seen (so a .ll test can't work). Should I make it a warning instead? That seems fairly permissive.
https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:23: namespace NaCl { On 2013/07/03 22:28:30, JF wrote: > On 2013/07/03 17:50:26, jvoung (cr) wrote: > > perhaps use allow lower case: namespace "nacl" > > > > We have namespace "naclbitc" and all lowercase would be more consistent. > > mseaborn had the same suggestion: using uppercase is actually more consistent > with other LLVM namespaces. I'd rename naclbitc instead, to match the rest of > the code. Well like a lot of LLVM conventions, it's not consistent. E.g., namespace dwarf, object, yaml, jitprofiling, cl, path, vs namespace X86Local, ... Anyway, something to clean up later. https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:176: unsigned alignmentFromPointer(const Value *Ptr) const { On 2013/07/03 22:28:30, JF wrote: > On 2013/07/03 20:14:20, jvoung (cr) wrote: > > On 2013/07/03 17:50:26, jvoung (cr) wrote: > > > Could this just be done by DataLayout's getPointerABIAlignment(), or some > > other > > > method? > > > > > > I guess that assumes our DataLayout uses natural alignment. > > > > Err... sorry not getPointerABIAlignment, but getABITypeAlignment of the > > pointed-to-type. > > I actually want the natural alignment for the type *pointed to*, AFAICT > getABITypeAlignment gives the alignment for pointer types. It does look like > DataLayout.h may have some equivalent, maybe I should do: > > getABIIntegerTypeAlignment(BitWidth) ? > > That seems overly complex, and doesn't match what RewriteAtomics.cpp does, so > I'd need to change that too. Opinion? > > Really all we want it natural alignment, which is what this does. At first I thought it would be good to reuse DataLayout, but it seems like this natural alignment requirement is independent of DataLayout so the current code is fine. https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... test/Transforms/NaCl/atomics.ll:17: ; Alignment is also expected to be at least natural alignment. On 2013/07/03 22:28:30, JF wrote: > On 2013/07/03 17:50:26, jvoung (cr) wrote: > > Is there a test for the alignment checks? > > How would you construct this test? It currently kills opt when a misaligned > load/store is seen (so a .ll test can't work). Should I make it a warning > instead? That seems fairly permissive. It doesn't need to be converted to a warning. If you didn't change anything, then you could at least make a separate test (e.g., atomics-alignment-error.ll), testing one of the cases. Otherwise, you could collect all the alignment errors, print them, then kill opt.
> 1. Bitfields are usually slower to access than discrete fields, because extra > bit extracting operations are required. > 2. Since this is the sandboxed translator we're talking about, the bitfields > will be expanded out anyways (they're inherently unportable), so you're not > saving any memory. Bitfield instructions, when properly aligned, can be quite fast. I though integer promotions was for instructions acting on non-canonical values, not structs? The layout won't change, just how values are obtained, and the backends should deal with them as it previously would have (if not them ABI stabilization has a non-zero cost and we should fix ISel to handle the new idiom, anything else is bad for our users). > Add comments that explain what each of these methods do. Do that for all public > (or protected interface) methods in classes/structs you define. Done. > Also, the LLVM style-guide (which we follow for within-LLVM localmods) is public > comments that describe classes, methods, etc, start with /// and refer to > parameters with "\p ParamName". Doxygen-y style (meh, but consistency is most > important). It's documented here: Done.
Reply to jvoung's comments. https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... File include/llvm/IR/NaClIntrinsics.h (right): https://codereview.chromium.org/17777004/diff/105001/include/llvm/IR/NaClIntr... include/llvm/IR/NaClIntrinsics.h:23: namespace NaCl { > Well like a lot of LLVM conventions, it's not consistent. > > E.g., namespace dwarf, object, yaml, jitprofiling, cl, path, vs namespace > X86Local, ... > > Anyway, something to clean up later. git grep -e "namespace [a-zA-Z0-9_][a-zA-Z0-9_]* *{" | cut -f2 -d: | sed 's/^ *namespace *//g' | sed 's/ *{.*//g' | sort -u | egrep "[A-Z]" | wc -l 109 git grep -e "namespace [a-zA-Z0-9_][a-zA-Z0-9_]* *{" | cut -f2 -d: | sed 's/^ *namespace *//g' | sed 's/ *{.*//g' | sort -u | egrep -v "[A-Z]" | wc -l 45 :-) https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/105001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:176: unsigned alignmentFromPointer(const Value *Ptr) const { > At first I thought it would be good to reuse DataLayout, but it seems like this > natural alignment requirement is independent of DataLayout so the current code > is fine. Agreed, I thought so too when you pointed me at it, but it seems overly complex for obtaining natural alignment. It relies on the target ABI, somewhat scary! https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... test/Transforms/NaCl/atomics.ll:17: ; Alignment is also expected to be at least natural alignment. > It doesn't need to be converted to a warning. If you didn't change anything, > then you could at least make a separate test (e.g., atomics-alignment-error.ll), > testing one of the cases. That would rely on checking that the return value is 1, and then piping stderr, no? Seems icky. > Otherwise, you could collect all the alignment errors, print them, then kill > opt. I'm not sure I understand this. I think the "right thing" would be to improve our error messages for ABI stability, as we started discussing this morning. That would allow us to pinpoint actual lines of code, and hopefully make these more testable.
https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... File test/Transforms/NaCl/atomics.ll (right): https://codereview.chromium.org/17777004/diff/105001/test/Transforms/NaCl/ato... test/Transforms/NaCl/atomics.ll:17: ; Alignment is also expected to be at least natural alignment. On 2013/07/03 23:43:18, JF wrote: > > It doesn't need to be converted to a warning. If you didn't change anything, > > then you could at least make a separate test (e.g., > atomics-alignment-error.ll), > > testing one of the cases. > > That would rely on checking that the return value is 1, and then piping stderr, > no? Seems icky. All of the "pnaclabi-check" tool's tests run "pnaclabi-check", which exits with 1 on error. All of those tests are able to check for multiple error messages ("CHECK: ... disallowed ..."). That seems like something llvm-lit would be able to handle. > > > Otherwise, you could collect all the alignment errors, print them, then kill > > opt. > > I'm not sure I understand this. > You could toggle a bool that is "ErrorEncountered", print the error message, and continue instead of exiting immediately. Since RewriteAtomics is a module pass, that can run over the whole module. Before returning from runOnModule, you can check if "ErrorEncountered" is true or false and report_fatal_error() or something if it's true. > > > I think the "right thing" would be to improve our error messages for ABI > stability, as we started discussing this morning. That would allow us to > pinpoint actual lines of code, and hopefully make these more testable. Sure, I agree that we should improve error messages, but we can also test that error messages are generated. The alignments are hidden from the intrinsics, so delaying the error checking to the pnacl abi verifier isn't possible?
> The alignments are hidden from the intrinsics, so delaying the error checking to > the pnacl abi verifier isn't possible? Correct, the intrinsics imply natural alignment.
Patches 28 and 29 fix some logic around alignment, handle float/double volatiles, and fix some of the value naming. PTAL.
looks pretty good, one question https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Rewr... File lib/Transforms/NaCl/RewriteAtomics.cpp (right): https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Rewr... lib/Transforms/NaCl/RewriteAtomics.cpp:280: PH.OriginalPET, PH.PET, Args); Does atomicrmw and cmpxchg need to handle float and double values too? (If the OriginalPET and PET don't match)?
LGTM with some small comments https://codereview.chromium.org/17777004/diff/108004/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/108004/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:71: NaCl::AtomicIntrinsics *AI; Name this member more descriptively. Also, use one of LLVM's smart pointers instead of manual memory management. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:50: bool Resolve(IntrinsicInst *Call) { Explain what Resolve (and DoResolve) returns https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:72: virtual Function *DoGetDeclaration() const = 0; I've just noticed that the naming convention is inconsistent here. We name functions & methods with lowercaseStartingCamel, per the LLVM coding guidelines. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:184: unsigned BitWidth = cast<IntegerType>( It's very hard to see what the casts refer to here. Break to separate assignments. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:186: return 1 << (CountTrailingZeros_32(BitWidth) - 3); Comment to explain the computation being done here
> https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Rewr... > lib/Transforms/NaCl/RewriteAtomics.cpp:280: PH.OriginalPET, PH.PET, Args); > Does atomicrmw and cmpxchg need to handle float and double values too? (If the > OriginalPET and PET don't match)? Both are handled here: lib/AsmParser/LLParser.cpp: return Error(NewLoc, "cmpxchg operand must be an integer"); lib/AsmParser/LLParser.cpp: return Error(ValLoc, "atomicrmw operand must be an integer"); I wrote the code in a way that tries to handle pointers in a instruction-neutral way as well as return values, but I didn't quite achieve the same result for incoming value parameters (it would be easy to do at Args creation time, but the code would never fire). If LLParser.cpp were to change the atomic intrinsics would fail to typecheck when getting non-integer parameters because the incoming type wouldn't be a valid overload (e.g. ``float atomicrmw op float*, float`` would become ``i32 nacl.atomic.rmw(op, i32*, float)`` which Intrinsics.td won't allow). One thing to note is that I also rely on LLVM's built-in type checker for instructions (in particular, bitcast) to catch anything weird: my code doesn't actually mention float and double, it just matches the incoming type to an integer type of the same size. In theory it should also handle any 8, 16, 32 and 64-bit non-integral types.
https://codereview.chromium.org/17777004/diff/108004/lib/Analysis/NaCl/PNaClA... File lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp (right): https://codereview.chromium.org/17777004/diff/108004/lib/Analysis/NaCl/PNaClA... lib/Analysis/NaCl/PNaClABIVerifyFunctions.cpp:71: NaCl::AtomicIntrinsics *AI; On 2013/07/09 17:17:19, eliben wrote: > Name this member more descriptively. Also, use one of LLVM's smart pointers > instead of manual memory management. Done. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:50: bool Resolve(IntrinsicInst *Call) { On 2013/07/09 17:17:19, eliben wrote: > Explain what Resolve (and DoResolve) returns Done. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:72: virtual Function *DoGetDeclaration() const = 0; On 2013/07/09 17:17:19, eliben wrote: > I've just noticed that the naming convention is inconsistent here. We name > functions & methods with lowercaseStartingCamel, per the LLVM coding guidelines. Done. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:184: unsigned BitWidth = cast<IntegerType>( On 2013/07/09 17:17:19, eliben wrote: > It's very hard to see what the casts refer to here. Break to separate > assignments. Done. https://codereview.chromium.org/17777004/diff/108004/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:186: return 1 << (CountTrailingZeros_32(BitWidth) - 3); On 2013/07/09 17:17:19, eliben wrote: > Comment to explain the computation being done here Done.
https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:50: /// Returns true if the Function was changes. s/changes/changed/ Below too https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:188: unsigned ByteWidth = 1 << (CountTrailingZeros_32(BitWidth) - 3); Ladies and gentlemen, I'm just a simple programmer. Your bit twiddling tricks anger and confuse me. Seriously, if there's a good reason why you're not dividing by 8 (the way it's done in many places in LLVM, please at least comment why you're doing it this way and what your specific constraints are.
https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... File lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp (right): https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:50: /// Returns true if the Function was changes. On 2013/07/11 14:56:05, eliben wrote: > s/changes/changed/ > > Below too Done. https://codereview.chromium.org/17777004/diff/167001/lib/Transforms/NaCl/Reso... lib/Transforms/NaCl/ResolvePNaClIntrinsics.cpp:188: unsigned ByteWidth = 1 << (CountTrailingZeros_32(BitWidth) - 3); On 2013/07/11 14:56:05, eliben wrote: > Ladies and gentlemen, I'm just a simple programmer. Your bit twiddling tricks > anger and confuse me. > > Seriously, if there's a good reason why you're not dividing by 8 (the way it's > done in many places in LLVM, please at least comment why you're doing it this > way and what your specific constraints are. Huh, duh. I should have noticed the simplification, the original code did more and needed this, but now it obviously doesn't.
I tracked down the last two failure in the GCC torture tests, I'll need to disable test 20010518-2.c in tools/toolchain_tester/known_failures_pnacl.txt (it does packed volatiles, which we could fix later but I'd like us to discuss options), and I submitted https://codereview.chromium.org/18502004/
Message was sent while issue was closed.
Committed patchset #35 manually as r4c1316e (presubmit successful). |
