Subzero. Enable Atomics in ARM.

Subzero. Enable Atomics in ARM.

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

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

[John, 2015-10-05 17:11:52]

Patchset #9 (id:160001) has been deleted

[John, 2015-10-05 17:12:08]

Patchset #9 (id:180001) has been deleted

[John, 2015-10-05 19:28:36]

jpp@chromium.org changed reviewers:
+ kschimpf@google.com, sehr@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2015-10-05 22:07:06]

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
File src/IceTargetLoweringARM32.cpp (right):

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
src/IceTargetLoweringARM32.cpp:2592: //     fake-use(addr, operand)  @ prevents
undesireable clobbering.
undesirable

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
src/IceTargetLoweringARM32.cpp:2614: if (DestTy != IceType_i64) {
(here and below)
I think instead of this:
  if (A != B)
    C;
  else
    D;

it's clearer to do this:
  if (A == B)
    D;
  else
    C;

so that there are essentially fewer double-negatives to work through when
reading the code.

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
File src/IceTargetLoweringARM32.h (right):

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
src/IceTargetLoweringARM32.h:393: // restriction is cleanly represented by
adding an "early definition of of
s/of of/of/
Also, should the double-quote be there?  (It's missing the closing
double-quote.)

https://chromiumcodereview.appspot.com/1369333003/diff/200001/src/IceTargetLo...
src/IceTargetLoweringARM32.h:402: Instr->setDestRedefined();
What do you think about adding Dest as another Src for the InstARM32Strex
instruction?  That would allow you not to have to call setDestRedefined() here. 
The fewer of those calls the better.

[John, 2015-10-06 12:03:38]

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.cpp (right):

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2592: //     fake-use(addr, operand)  @ prevents
undesireable clobbering.
On 2015/10/05 22:07:06, stichnot wrote:
> undesirable

Done.

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.cpp:2614: if (DestTy != IceType_i64) {
On 2015/10/05 22:07:06, stichnot wrote:
> (here and below)
> I think instead of this:
>   if (A != B)
>     C;
>   else
>     D;
> 
> it's clearer to do this:
>   if (A == B)
>     D;
>   else
>     C;
> 
> so that there are essentially fewer double-negatives to work through when
> reading the code.

Done here, and elsewhere.

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
File src/IceTargetLoweringARM32.h (right):

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:393: // restriction is cleanly represented by
adding an "early definition of of
On 2015/10/05 22:07:06, stichnot wrote:
> s/of of/of/
> Also, should the double-quote be there?  (It's missing the closing
> double-quote.)

Done.

https://codereview.chromium.org/1369333003/diff/200001/src/IceTargetLoweringA...
src/IceTargetLoweringARM32.h:402: Instr->setDestRedefined();
On 2015/10/05 22:07:06, stichnot wrote:
> What do you think about adding Dest as another Src for the InstARM32Strex
> instruction?  That would allow you not to have to call setDestRedefined()
here. 
> The fewer of those calls the better.

Well, if the method was still named nonkillable, sure, I'd do it. Now that it is
named redefined it precisely tells what's happening here. I'd rather leave this
as is. Besides, I don't want to "leak" liveness requirements down to the InstARM
layer.

[Jim Stichnoth, 2015-10-06 13:07:41]

lgtm

[John, 2015-10-06 13:54:47]

Committed patchset #10 (id:220001) manually as
578f1161ad7a3ef98c90d88e5734940523d1f8e0 (presubmit successful).