Subzero: Support non sequentially consistent memory orderings for atomic ops.

Subzero: Support non sequentially consistent memory orderings for atomic ops.

The actual code lowering is unchanged, but the validation is made less strict to allow the additional orderings.

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

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=1c335ef464bfb1232e250c537a2f276ca5980857

[Jim Stichnoth, 2015-03-17 14:40:19]

stichnot@chromium.org changed reviewers:
+ jfb@chromium.org, jvoung@chromium.org, kschimpf@google.com,
mtrofin@chromium.org

[JF, 2015-03-17 16:00:08]

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:237: uint64_t OrderOther) {
Rename to isMemoryOrderValid?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
unreachable, return false *and* break! Porque no les tres?

Can it be reached if the input file is bad? Or does the decoder prevent this?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:250: return true;
You have to reject relaxed and consume, though you should do this at the very
top (PNaCl restriction, not C++ restriction).

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:256: case Intrinsics::MemoryOrderAcquireRelease:
Additional PNaCl restriction: reject relaxed and consume.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:262: OrderOther == Intrinsics::MemoryOrderRelaxed)
This should be:
  Success == Release && Failure != Relaxed

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:284: return false;
Is this reachable?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:286: return Order ==
Intrinsics::MemoryOrderSequentiallyConsistent;
This definitely isn't reachable!

[Jim Stichnoth, 2015-03-17 16:44:56]

Thanks!

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:237: uint64_t OrderOther) {
On 2015/03/17 16:00:07, JF wrote:
> Rename to isMemoryOrderValid?

Done.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
On 2015/03/17 16:00:07, JF wrote:
> unreachable, return false *and* break! Porque no les tres?

20 goto 10

But yeah, break was left over from a previous iteration...

Leaving the "return false" because (as I understand it) llvm_unreachable()
doesn't guarantee an abort.

> 
> Can it be reached if the input file is bad? Or does the decoder prevent this?

Given that nacl-atomic-errors.ll explicitly tests these errors, I would say the
parser is not validating this...

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:250: return true;
On 2015/03/17 16:00:07, JF wrote:
> You have to reject relaxed and consume, though you should do this at the very
> top (PNaCl restriction, not C++ restriction).

Oops, done.

BTW, I should have brought this up in issue 4029, but is it really OK that we
are taking C++11 rules and applying them to all languages/compilers that target
PNaCl?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:256: case Intrinsics::MemoryOrderAcquireRelease:
On 2015/03/17 16:00:07, JF wrote:
> Additional PNaCl restriction: reject relaxed and consume.

Done.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:262: OrderOther == Intrinsics::MemoryOrderRelaxed)
On 2015/03/17 16:00:07, JF wrote:
> This should be:
>   Success == Release && Failure != Relaxed

Done.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:284: return false;
On 2015/03/17 16:00:08, JF wrote:
> Is this reachable?

Done.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:286: return Order ==
Intrinsics::MemoryOrderSequentiallyConsistent;
On 2015/03/17 16:00:07, JF wrote:
> This definitely isn't reachable!

Oops, I forgot to remove the old code!

[JF, 2015-03-17 16:58:53]

The testing isn't very thorough... Do you think Subzero could be compared
against the translator to check that for each of the intrinsics they allow the
same things?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
On 2015/03/17 16:44:56, stichnot wrote:
> On 2015/03/17 16:00:07, JF wrote:
> > unreachable, return false *and* break! Porque no les tres?
> 
> 20 goto 10
> 
> But yeah, break was left over from a previous iteration...
> 
> Leaving the "return false" because (as I understand it) llvm_unreachable()
> doesn't guarantee an abort.

It's truly unreachable if you whitelist above :-)


> > Can it be reached if the input file is bad? Or does the decoder prevent
this?
> 
> Given that nacl-atomic-errors.ll explicitly tests these errors, I would say
the
> parser is not validating this...

It's between you and Karl, but shouldn't the reader be the one deciding what's
invalid?

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:250: return true;
On 2015/03/17 16:44:56, stichnot wrote:
> On 2015/03/17 16:00:07, JF wrote:
> > You have to reject relaxed and consume, though you should do this at the
very
> > top (PNaCl restriction, not C++ restriction).
> 
> Oops, done.
> 
> BTW, I should have brought this up in issue 4029, but is it really OK that we
> are taking C++11 rules and applying them to all languages/compilers that
target
> PNaCl?

I think so: they're generally usable as a base to build other languages on top.
It would be nice to have relaxed too, I'm being pretty conservative with that.

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:245: }
I'd do the opposite and whitelist acquire/release/acq_rel/seq_cst: the file
format could contain any other number and you don't want to let them through.

https://codereview.chromium.org/1017453007/diff/60001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-atomic-errors.ll (right):

https://codereview.chromium.org/1017453007/diff/60001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-errors.ll:81: ;;; All modes 1:6 allowed.
In C++ they are, but not in PNaCl.

[Jim Stichnoth, 2015-03-17 18:42:05]

I pulled in the PNaCl abi-atomics.ll test for more comprehensive testing.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
On 2015/03/17 16:58:53, JF wrote:
> On 2015/03/17 16:44:56, stichnot wrote:
> > On 2015/03/17 16:00:07, JF wrote:
> > > unreachable, return false *and* break! Porque no les tres?
> > 
> > 20 goto 10
> > 
> > But yeah, break was left over from a previous iteration...
> > 
> > Leaving the "return false" because (as I understand it) llvm_unreachable()
> > doesn't guarantee an abort.
> 
> It's truly unreachable if you whitelist above :-)

Actually the ID can be for any intrinsic, not just atomic intrinsics.

> 
> 
> > > Can it be reached if the input file is bad? Or does the decoder prevent
> this?
> > 
> > Given that nacl-atomic-errors.ll explicitly tests these errors, I would say
> the
> > parser is not validating this...
> 
> It's between you and Karl, but shouldn't the reader be the one deciding what's
> invalid?

I agree that ultimately the reader should be the gatekeeper...

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:245: }
On 2015/03/17 16:58:53, JF wrote:
> I'd do the opposite and whitelist acquire/release/acq_rel/seq_cst: the file
> format could contain any other number and you don't want to let them through.

Done.

https://codereview.chromium.org/1017453007/diff/60001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-atomic-errors.ll (right):

https://codereview.chromium.org/1017453007/diff/60001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-errors.ll:81: ;;; All modes 1:6 allowed.
On 2015/03/17 16:58:53, JF wrote:
> In C++ they are, but not in PNaCl.

Done.

[jvoung (off chromium), 2015-03-17 19:05:22]

Small suggestion only.

Also agree that at some point this should be moved to somewhere common like the
reader, rather than be per-target...

https://codereview.chromium.org/1017453007/diff/100001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/100001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:259: case Intrinsics::MemoryOrderInvalid:
Maybe call out

case Intrinsics::MemoryOrderInvalid:
case Intrinsics::MemoryOrderNum:

as being under the "return false" case in the earlier check "// Reject orderings
not allowed in PNaCl." and then it doesn't need to be checked under the
per-intrinsic ID checks.

[Jim Stichnoth, 2015-03-17 21:05:11]

https://codereview.chromium.org/1017453007/diff/100001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/100001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:259: case Intrinsics::MemoryOrderInvalid:
On 2015/03/17 19:05:21, jvoung wrote:
> Maybe call out
> 
> case Intrinsics::MemoryOrderInvalid:
> case Intrinsics::MemoryOrderNum:
> 
> as being under the "return false" case in the earlier check "// Reject
orderings
> not allowed in PNaCl." and then it doesn't need to be checked under the
> per-intrinsic ID checks.

Done.

[JF, 2015-03-17 21:18:15]

There's also no test that non-constant-int memory order are rejected.

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
> > > > Can it be reached if the input file is bad? Or does the decoder prevent
> > this?
> > > 
> > > Given that nacl-atomic-errors.ll explicitly tests these errors, I would
say
> > the
> > > parser is not validating this...
> > 
> > It's between you and Karl, but shouldn't the reader be the one deciding
what's
> > invalid?
> 
> I agree that ultimately the reader should be the gatekeeper...

Bug?

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:268: OrderOther != Intrinsics::MemoryOrderRelaxed)
I would leave this in, otherwise when we add relaxed we won't be doing the right
thing.

https://codereview.chromium.org/1017453007/diff/120001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/120001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:259: // combinations for cmpxchg.
This code should only deal with what's invalid for C++, not PNaCl.

[JF, 2015-03-17 21:18:15]

There's also no test that non-constant-int memory order are rejected.

[Jim Stichnoth, 2015-03-18 00:00:25]

Added a test for non-constant integer memory ordering.

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/60001/src/IceIntrinsics.cpp#n...
src/IceIntrinsics.cpp:268: OrderOther != Intrinsics::MemoryOrderRelaxed)
On 2015/03/17 21:18:14, JF wrote:
> I would leave this in, otherwise when we add relaxed we won't be doing the
right
> thing.

OK, yeah, that optimization did seem too good to be true...

https://codereview.chromium.org/1017453007/diff/120001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/120001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:259: // combinations for cmpxchg.
On 2015/03/17 21:18:14, JF wrote:
> This code should only deal with what's invalid for C++, not PNaCl.

Done.

[JF, 2015-03-18 06:26:30]

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
On 2015/03/17 21:18:14, JF wrote:
> > > > > Can it be reached if the input file is bad? Or does the decoder
prevent
> > > this?
> > > > 
> > > > Given that nacl-atomic-errors.ll explicitly tests these errors, I would
> say
> > > the
> > > > parser is not validating this...
> > > 
> > > It's between you and Karl, but shouldn't the reader be the one deciding
> what's
> > > invalid?
> > 
> > I agree that ultimately the reader should be the gatekeeper...
> 
> Bug?

No bug for this? :(

https://codereview.chromium.org/1017453007/diff/140001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/140001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:273: case MemoryOrderSequentiallyConsistent:
This is wrong, order can also be relaxed or consume. The rule is:

// C++11 [atomics.types.operations.req]: The failure argument shall not be
// release nor acq_rel. The failure argument shall be no stronger than the
// success argument.
// Where the partial ordering is:
//   relaxed < consume < acquire           < acq_rel < seq_cst
//   relaxed <                     release < acq_rel < seq_cst

See:
 
https://codereview.chromium.org/791053006/diff/220001/lib/Analysis/NaCl/PNaCl...

[Jim Stichnoth, 2015-03-18 13:50:15]

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/1/src/IceIntrinsics.cpp#newco...
src/IceIntrinsics.cpp:246: break;
On 2015/03/18 06:26:30, JF wrote:
> On 2015/03/17 21:18:14, JF wrote:
> > > > > > Can it be reached if the input file is bad? Or does the decoder
> prevent
> > > > this?
> > > > > 
> > > > > Given that nacl-atomic-errors.ll explicitly tests these errors, I
would
> > say
> > > > the
> > > > > parser is not validating this...
> > > > 
> > > > It's between you and Karl, but shouldn't the reader be the one deciding
> > what's
> > > > invalid?
> > > 
> > > I agree that ultimately the reader should be the gatekeeper...
> > 
> > Bug?
> 
> No bug for this? :(

Done.

https://codereview.chromium.org/1017453007/diff/140001/src/IceIntrinsics.cpp
File src/IceIntrinsics.cpp (right):

https://codereview.chromium.org/1017453007/diff/140001/src/IceIntrinsics.cpp#...
src/IceIntrinsics.cpp:273: case MemoryOrderSequentiallyConsistent:
On 2015/03/18 06:26:30, JF wrote:
> This is wrong, order can also be relaxed or consume. The rule is:
> 
> // C++11 [atomics.types.operations.req]: The failure argument shall not be
> // release nor acq_rel. The failure argument shall be no stronger than the
> // success argument.
> // Where the partial ordering is:
> //   relaxed < consume < acquire           < acq_rel < seq_cst
> //   relaxed <                     release < acq_rel < seq_cst
> 
> See:
>  
>
https://codereview.chromium.org/791053006/diff/220001/lib/Analysis/NaCl/PNaCl...

But but, that's covered by the isMemoryOrderValidPNaCl(OrderOther) test, and the
comprehensive tests all pass, and...

Oh, OK, separation of PNaCl and C++11 requirements.  Done.

[JF, 2015-03-18 15:28:25]

lgtm

[Jim Stichnoth, 2015-03-18 16:01:58]

Committed patchset #9 (id:150001) manually as
1c335ef464bfb1232e250c537a2f276ca5980857 (presubmit successful).