[Subzero][MIPS] Implements atomic intrinsics for MIPS32

[Subzero][MIPS] Implements atomic intrinsics for MIPS32

R=stichnot@chromium.org

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

[sagar.thakur, 2016-11-16 13:42:26]

sagar.thakur@imgtec.com changed reviewers:
+ eholk@chromium.org, jaydeep.patil@imgtec.com, jpp@chromium.org,
kschimpf@google.com, stichnot@chromium.org

[sagar.thakur, 2016-11-16 13:42:27]

Results of test_sync_atomic:

O2 mode: TotalTests=240264 Passes=240264 Failures=0
Om1 mode: TotalTests=240264 Passes=236670 Failures=3594

In Om1 mode failures are due to wrong reference output generated by llc. The
problem is that llc does not sign extend argument values properly.

I have added some lit tests too. But haven't uploaded them here because I need
to do some more changes in the lit test. Will upload it as soon as I am done
with those changes.

[Jim Stichnoth, 2016-11-17 05:35:39]

https://codereview.chromium.org/2504253002/diff/1/src/IceAssemblerMIPS32.cpp
File src/IceAssemblerMIPS32.cpp (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:1138: IValueT Opcode = 0x0000000f;
static constexpr like the others?

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h#newcode611
src/IceInstMIPS32.h:611: Str << "\t"
While you're at it...
Instead of
  Str << "\t" << "lb" << "\t";
Use string concatenation for smaller&faster code:
  Str << "\t" "lb" "\t";

(here and below)

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h#newcode975
src/IceInstMIPS32.h:975: const char *Opcode = "sync";
Opcode is usually a static const char* (which you then have to separately define
in the .cpp file).

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4256: void
TargetMIPS32::createArithInst(uint32_t Operation, Variable *Dest,
Can you declare Operation as Intrinsics::IntrinsicID instead of uint32_t?

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4286: switch (Instr->getIntrinsicInfo().ID) {
switch (ID) {

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4312: // Adding a fake-use of T to ensure the
atomic load is not removed
Reflow this and other comments to 80-col

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4333: Func->setError("Unexpected memory ordering
for AtomicLoad");
AtomicStore

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4344: uint64_t Lower32Bits = Value & 0XFFFFFFFF;
lowercase 'x' for consistency
(here and below)

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4511: Func->setError("Unexpected memory ordering
for AtomicCmpxchg");
AtomicRMW

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4525: uint32_t Operation =
static_cast<uint32_t>(
Can it be like this:

  auto Operation = static_cast<Intrinsics::IntrinsicID>(

[sagar.thakur, 2016-11-17 10:57:29]

Address review comments and re-structured code in lowerIntrinsics.

https://codereview.chromium.org/2504253002/diff/1/src/IceAssemblerMIPS32.cpp
File src/IceAssemblerMIPS32.cpp (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceAssemblerMIPS32.cpp#...
src/IceAssemblerMIPS32.cpp:1138: IValueT Opcode = 0x0000000f;
On 2016/11/17 05:35:38, Jim Stichnoth wrote:
> static constexpr like the others?

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h
File src/IceInstMIPS32.h (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h#newcode611
src/IceInstMIPS32.h:611: Str << "\t"
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> While you're at it...
> Instead of
>   Str << "\t" << "lb" << "\t";
> Use string concatenation for smaller&faster code:
>   Str << "\t" "lb" "\t";
> 
> (here and below)

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceInstMIPS32.h#newcode975
src/IceInstMIPS32.h:975: const char *Opcode = "sync";
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> Opcode is usually a static const char* (which you then have to separately
define
> in the .cpp file).

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
File src/IceTargetLoweringMIPS32.cpp (right):

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4256: void
TargetMIPS32::createArithInst(uint32_t Operation, Variable *Dest,
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> Can you declare Operation as Intrinsics::IntrinsicID instead of uint32_t?

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4286: switch (Instr->getIntrinsicInfo().ID) {
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> switch (ID) {

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4312: // Adding a fake-use of T to ensure the
atomic load is not removed
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> Reflow this and other comments to 80-col

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4333: Func->setError("Unexpected memory ordering
for AtomicLoad");
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> AtomicStore

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4344: uint64_t Lower32Bits = Value & 0XFFFFFFFF;
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> lowercase 'x' for consistency
> (here and below)

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4511: Func->setError("Unexpected memory ordering
for AtomicCmpxchg");
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> AtomicRMW

Done.

https://codereview.chromium.org/2504253002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:4525: uint32_t Operation =
static_cast<uint32_t>(
On 2016/11/17 05:35:39, Jim Stichnoth wrote:
> Can it be like this:
> 
>   auto Operation = static_cast<Intrinsics::IntrinsicID>(

Done.

[Jim Stichnoth, 2016-11-18 20:15:16]

LGTM.  (I went ahead and fixed the formatting comment.)

https://codereview.chromium.org/2504253002/diff/40001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll (right):

https://codereview.chromium.org/2504253002/diff/40001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll:29: ; RUN: %if
--need=allow_dump --need=target_MIPS32 --command %p2i --filetype=asm \
80-col, here and below

[Jim Stichnoth, 2016-11-18 20:15:49]

Description was changed from

==========
[Subzero][MIPS] Implements atomic intrinsics for MIPS32
==========

to

==========
[Subzero][MIPS] Implements atomic intrinsics for MIPS32

R=stichnot@chromium.org

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

[Jim Stichnoth, 2016-11-18 20:15:50]

Committed patchset #3 (id:40001) manually as
becb85f1fa938e3046576ad2c6e9abe60b7288e7 (presubmit successful).