[SubZero] Generate spin-lock for atomic load store

[SubZero] Generate spin-lock for atomic load store

The patch generates LL-SC sequence for atomic load store operations.
64-bit atomic operations are lowered to __sync_*_8 functions.

R=stichnot@chromium.org

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

[jaydeep.patil, 2017-02-07 13:17:38]

Description was changed from

==========
[SubZero] Generate spin-lock for atomic load store

The patch generates LL-SC sequence for atomic load store operations.
64-bit atomic operations are lowered to __sync_*_8 functions.
==========

to

==========
[SubZero] Generate spin-lock for atomic load store

The patch generates LL-SC sequence for atomic load store operations.
64-bit atomic operations are lowered to __sync_*_8 functions.
==========

[jaydeep.patil, 2017-02-07 13:17:38]

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

[Jim Stichnoth, 2017-02-07 17:45:43]

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLowering.def#n...
src/IceTargetLowering.def:48: X(sync_f_n_add_8,     "__sync_fetch_and_add_8")   
                          \
Sort these lines alphabetically, for consistency.

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

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:635: auto *ZeroConst =
ConstantInteger64::create(Ctx, IceType_i64, 0);
Here you may want to use GlobalContext::getConstantZero(IceType_i64).

Also, at some point in the x86/ARM code, we started a convention where the
variable name for a known constant like this would be "_0".  There's one
instance already in the MIPS code, "_4".  You might consider using _0 instead of
ZeroConst.

[jaydeep.patil, 2017-02-08 05:08:25]

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLowering.def#n...
src/IceTargetLowering.def:48: X(sync_f_n_add_8,     "__sync_fetch_and_add_8")   
                          \
On 2017/02/07 17:45:43, Jim Stichnoth wrote:
> Sort these lines alphabetically, for consistency.

Done.

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

https://codereview.chromium.org/2682673002/diff/1/src/IceTargetLoweringMIPS32...
src/IceTargetLoweringMIPS32.cpp:635: auto *ZeroConst =
ConstantInteger64::create(Ctx, IceType_i64, 0);
On 2017/02/07 17:45:43, Jim Stichnoth wrote:
> Here you may want to use GlobalContext::getConstantZero(IceType_i64).
> 
> Also, at some point in the x86/ARM code, we started a convention where the
> variable name for a known constant like this would be "_0".  There's one
> instance already in the MIPS code, "_4".  You might consider using _0 instead
of
> ZeroConst.

Done.

[Jim Stichnoth, 2017-02-10 07:16:19]

https://codereview.chromium.org/2682673002/diff/20001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/2682673002/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:48: X(sync_f_n_add_8,     "__sync_fetch_and_add_8")   
                          \
Unfortunately I just discovered a problem with this way of adding MIPS-specific
helper functions.

When I run "make presubmit", the sandboxed cross tests fail because these
symbols are undefined.

The problem is that with -filetype=obj, explicit references to all these symbols
are added to the ELF object file.  Even though they are not actually used, the
pnacl-ld linker gives an error when no definitions are provided.

The solution is to look up these symbols only as needed, instead of adding them
to the unconditionally-instantiated RuntimeHelperFunc[] table at startup.

Look at the uses of __aeabi_read_tp as an example of how this is done for that
ARM-specific symbol.

Also, please document these functions at the bottom of runtime/szrt.c.  Maybe
just put them under a new section e.g.:

// MIPS runtime library:
//   __sync_fetch_and_add_8
//   __sync_fetch_and_and_8
//   ...

[jaydeep.patil, 2017-02-10 08:31:24]

https://codereview.chromium.org/2682673002/diff/20001/src/IceTargetLowering.def
File src/IceTargetLowering.def (right):

https://codereview.chromium.org/2682673002/diff/20001/src/IceTargetLowering.d...
src/IceTargetLowering.def:48: X(sync_f_n_add_8,     "__sync_fetch_and_add_8")   
                          \
On 2017/02/10 07:16:19, Jim Stichnoth wrote:
> Unfortunately I just discovered a problem with this way of adding
MIPS-specific
> helper functions.
> 
> When I run "make presubmit", the sandboxed cross tests fail because these
> symbols are undefined.
> 
> The problem is that with -filetype=obj, explicit references to all these
symbols
> are added to the ELF object file.  Even though they are not actually used, the
> pnacl-ld linker gives an error when no definitions are provided.
> 
> The solution is to look up these symbols only as needed, instead of adding
them
> to the unconditionally-instantiated RuntimeHelperFunc[] table at startup.
> 
> Look at the uses of __aeabi_read_tp as an example of how this is done for that
> ARM-specific symbol.
> 
> Also, please document these functions at the bottom of runtime/szrt.c.  Maybe
> just put them under a new section e.g.:
> 
> // MIPS runtime library:
> //   __sync_fetch_and_add_8
> //   __sync_fetch_and_and_8
> //   ...

Done.

[Jim Stichnoth, 2017-02-10 14:45:31]

lgtm

[Jim Stichnoth, 2017-02-10 14:45:45]

Description was changed from

==========
[SubZero] Generate spin-lock for atomic load store

The patch generates LL-SC sequence for atomic load store operations.
64-bit atomic operations are lowered to __sync_*_8 functions.
==========

to

==========
[SubZero] Generate spin-lock for atomic load store

The patch generates LL-SC sequence for atomic load store operations.
64-bit atomic operations are lowered to __sync_*_8 functions.

R=stichnot@chromium.org

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

[Jim Stichnoth, 2017-02-10 14:45:47]

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