Subzero: lower the rest of the atomic operations.

Subzero: lower the rest of the atomic operations.

64-bit ops are expanded via a cmpxchg8b loop.

64/32-bit and/or/xor are also expanded into a cmpxchg /
cmpxchg8b loop.

Add a cross test for atomic RMW operations and
compare and swap.

Misc: Test that atomic.is.lock.free can be optimized out if result is ignored.

TODO:
* optimize compare and swap with compare+branch further down
instruction stream.

* optimize atomic RMW when the return value is ignored
(adds a locked field to binary ops though).

* We may want to do some actual target-dependent basic
block splitting + expansion (the instructions inserted by
the expansion must reference the pre-colored registers,
etc.). Otherwise, we are currently getting by with modeling
the extended liveness of the variables used in the loops
using fake uses.

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

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

[jvoung (off chromium), 2014-07-07 17:31:16]

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2719: if (hasFramePointer())
split out -- but currently here so that my tests pass

[Jim Stichnoth, 2014-07-08 04:50:20]

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic.cpp:13: if (fetch_first) return
__sync_fetch_and_##inst(ptr, a);            \
Violation (here and below) of Google style guide. :)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.def (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic.def:4: #define XSTR(s) STR(s)
XSTR() is not used anywhere, consider removing it?

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic_main.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:2: --prefix=Subzero_
--output=test_sync_atomic */
Add --crosstest-bitcode=0 ?

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:43: struct AtomicLocs {
Maybe change to:
  struct { ... } AtomicLocs;
since the struct name isn't used elsewhere.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:70: }                                       
                                 \
Put comma on the same line, as above?

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:332: static InstX8632Unaryop *create(Cfg *Func, Variable
*SrcDest) {
An instruction like this is valid x86:
  neg [eax+4*ecx]
but this implementation of UnaryOp only allows these forms:
  neg eax
  neg [esp+16]
since SrcDest is a Variable.  That's probably sufficient for our needs, but
ought to be documented.

Another possibility might be to declare SrcDest as Operand*, and set
Dest=llvm::dyn_cast<Variable>(SrcDest), but I don't know if that would create
any liveness problems.  (BTW, I see that you've done exactly that with cmpxchg.)

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: // possibly changed.
Would it be simpler to swap the roles of Eax and DestOrAddr with respect to the
Dest operand?  That way, the instruction would always have a non-NULL Dest, and
the lowering code could conditionally create the FakeDef if DestOrAddr is a
Variable.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:581: // <m64> must be a memory operand.
Document the user's responsibility to add appropriate FakeDefs.

BTW, in the style of some other 64-bit integer instructions, you could also
define eax to be the Dest variable, and then only have to create a FakeDef for
edx.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:950: static InstX8632Xchg *create(Cfg *Func, Operand *Dest,
Variable *Source) {
Can we structure this so that Dest is the Variable and Source is the Operand?

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1823: // optimized out the later compare, and
fuse with the later branch.
optimized --> optimize

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2169: // choosing eax for both. Work around this
for now, but forcing Addr
I'd like to understand more about this.  It sounds like something is going wrong
with register allocation (maybe the Om1 hack that passes for register
allocation?) or with lowering.

Also, "but" --> "by" ?

Same comments on the same comment below.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:180: void _cmpxchg(Operand *DestOrAddr, Variable
*Eax, Variable *Desired,
This is probably not worth doing now, but in the future it might be nice to be
able to write stylized lowering code like:

  _lock(_cmpxchg(...));

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:302: Context.insert(InstFakeDef::create(Func,
Src));
If (as suggested earlier) Dest becomes a Variable and Src becomes an Operand,
then the FakeDef would be conditionally inserted.

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

https://codereview.chromium.org/362463002/diff/350013/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll:233: ; CHECK: lock cmpxchg8b
qword ptr [e{{.[^x]}}]
For the cmpxchg8b tests, it would be good to check for "push ebx" in the prolog.
 I think that something would have to go very wrong to trigger that failure, but
still...

[jvoung (off chromium), 2014-07-08 18:14:07]

Thanks Jim, a couple of replies first before I address the rest of the comments.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic.cpp:13: if (fetch_first) return
__sync_fetch_and_##inst(ptr, a);            \
On 2014/07/08 04:50:19, stichnot wrote:
> Violation (here and below) of Google style guide. :)
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

Done.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.def (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic.def:4: #define XSTR(s) STR(s)
On 2014/07/08 04:50:19, stichnot wrote:
> XSTR() is not used anywhere, consider removing it?

Done.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
File crosstest/test_sync_atomic_main.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:2: --prefix=Subzero_
--output=test_sync_atomic */
On 2014/07/08 04:50:19, stichnot wrote:
> Add --crosstest-bitcode=0 ?

Done.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:43: struct AtomicLocs {
On 2014/07/08 04:50:19, stichnot wrote:
> Maybe change to:
>   struct { ... } AtomicLocs;
> since the struct name isn't used elsewhere.

Done.

https://codereview.chromium.org/362463002/diff/350013/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:70: }                                       
                                 \
On 2014/07/08 04:50:19, stichnot wrote:
> Put comma on the same line, as above?

Done.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:332: static InstX8632Unaryop *create(Cfg *Func, Variable
*SrcDest) {
On 2014/07/08 04:50:19, stichnot wrote:
> An instruction like this is valid x86:
>   neg [eax+4*ecx]
> but this implementation of UnaryOp only allows these forms:
>   neg eax
>   neg [esp+16]
> since SrcDest is a Variable.  That's probably sufficient for our needs, but
> ought to be documented.
> 
> Another possibility might be to declare SrcDest as Operand*, and set
> Dest=llvm::dyn_cast<Variable>(SrcDest), but I don't know if that would create
> any liveness problems.  (BTW, I see that you've done exactly that with
cmpxchg.)

Okay, I think I will try making it a more generic Operand.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: // possibly changed.
On 2014/07/08 04:50:19, stichnot wrote:
> Would it be simpler to swap the roles of Eax and DestOrAddr with respect to
the
> Dest operand?  That way, the instruction would always have a non-NULL Dest,
and
> the lowering code could conditionally create the FakeDef if DestOrAddr is a
> Variable.

I think this is similar to Xchg. See below?

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:950: static InstX8632Xchg *create(Cfg *Func, Operand *Dest,
Variable *Source) {
On 2014/07/08 04:50:19, stichnot wrote:
> Can we structure this so that Dest is the Variable and Source is the Operand?

I think the motivation for this was to make it like "Store" in the sense that if
Dest is not a variable, then Dest is NULL and it cannot be DCE'ed when the Dest
is actually a dead variable (it shouldn't be because Source might be memory and
we don't know if that is a dead store).

If we flip Dest and Source, is there another way to prevent it from DCE'ed? Just
mark the instruction as HasSideEffects when the source is memory?

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2169: // choosing eax for both. Work around this
for now, but forcing Addr
On 2014/07/08 04:50:19, stichnot wrote:
> I'd like to understand more about this.  It sounds like something is going
wrong
> with register allocation (maybe the Om1 hack that passes for register
> allocation?) or with lowering.
> 
> Also, "but" --> "by" ?
> 
> Same comments on the same comment below.

The problem was actually with O2 register allocation.

I think what happens is the following:

- Reg alloc notices that LegalPtr has an overlapping liverange w/ T_eax. That
part is good.
- However, LegalPtr has a preferred register, namely, whatever register was
allocated for the earlier "Ptr" variable.
- "Ptr" *does not* have a live range that overlaps T_eax (it ends when we
convert Ptr to LegalPtr).
- Ptr gets allocated to Eax.
- Since LegalPtr prefers whatever reg Ptr gets, then it also gets Eax.
- I don't think there is any additional checking for overlap w/ a pre-colored
register, once there is a preferred register.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:180: void _cmpxchg(Operand *DestOrAddr, Variable
*Eax, Variable *Desired,
On 2014/07/08 04:50:19, stichnot wrote:
> This is probably not worth doing now, but in the future it might be nice to be
> able to write stylized lowering code like:
> 
>   _lock(_cmpxchg(...));

Yes that would be pretty sweet syntax.

The issue is what should _lock(InstX8632Lockable) do?  I could make
InstX8632Lockable have a "setLocked()" method, and toggle w/ _lock().

What LLVM does is make LOCK a completely separate instruction. So it would be
like Context.insert(LOCK), but then the syntax would be more like _lock();
_cmpxchg(...);.

I didn't like that because it separates the prefix from the instruction, which
means that say, when you want to know if the instruction is locked (for whatever
reason), you have to look backward a bit.

[jvoung (off chromium), 2014-07-09 17:07:56]

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:332: static InstX8632Unaryop *create(Cfg *Func, Variable
*SrcDest) {
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > An instruction like this is valid x86:
> >   neg [eax+4*ecx]
> > but this implementation of UnaryOp only allows these forms:
> >   neg eax
> >   neg [esp+16]
> > since SrcDest is a Variable.  That's probably sufficient for our needs, but
> > ought to be documented.
> > 
> > Another possibility might be to declare SrcDest as Operand*, and set
> > Dest=llvm::dyn_cast<Variable>(SrcDest), but I don't know if that would
create
> > any liveness problems.  (BTW, I see that you've done exactly that with
> cmpxchg.)
> 
> Okay, I think I will try making it a more generic Operand.

Done.

Not sure about liveness either, but Dest would only be null if it's not a
Variable, and then that would mean memory. In that case, it's like a store,
where Dest is also null?

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: // possibly changed.
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > Would it be simpler to swap the roles of Eax and DestOrAddr with respect to
> the
> > Dest operand?  That way, the instruction would always have a non-NULL Dest,
> and
> > the lowering code could conditionally create the FakeDef if DestOrAddr is a
> > Variable.
> 
> I think this is similar to Xchg. See below?

Okay, this is slightly different from Xchg, in that it has an explicit Locked
parameter.

As we discussed, if DestOrAddr is a memory operand, and we don't want it to be
DCE'ed when Eax is unused. Locked=true helps because it marks the instruction as
HasSideEffects. It doesn't help when Locked=false (the caller has the option to
make that mistake)... I'm not sure sure how likely that is. Interestingly
enough, in contrast, Xchg seems more useful to use not-Locked, though Locking is
automatic there (can be used for 16-bit bswap w/ two register operands)...

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:581: // <m64> must be a memory operand.
On 2014/07/08 04:50:19, stichnot wrote:
> Document the user's responsibility to add appropriate FakeDefs.
> 
> BTW, in the style of some other 64-bit integer instructions, you could also
> define eax to be the Dest variable, and then only have to create a FakeDef for
> edx.

Documented.

The other 64-bit integer instruction touching eax would be mul?

I feel that this a bit different from that, and this is more like a store
instead.

For mul, what's modified is registers (edx:eax), so it makes sense to just put
Eax in the Dest so that you don't need two FakeDefs and you have some Dest'ed
variable to chain the edx FakeDef against. However, cmpxchg8b, like cmpxchg,
will modify memory as well and the choice of Locked=true/false is again up to
the user.

Maybe for cmpxchg8b it will always "HaveSideEffects", because one of the dests
is always memory (in contrast to cmpxchg, where there is a choice).

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:1823: // optimized out the later compare, and
fuse with the later branch.
On 2014/07/08 04:50:19, stichnot wrote:
> optimized --> optimize

Done.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2719: if (hasFramePointer())
On 2014/07/07 17:31:16, jvoung wrote:
> split out -- but currently here so that my tests pass

Done.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:180: void _cmpxchg(Operand *DestOrAddr, Variable
*Eax, Variable *Desired,
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > This is probably not worth doing now, but in the future it might be nice to
be
> > able to write stylized lowering code like:
> > 
> >   _lock(_cmpxchg(...));
> 
> Yes that would be pretty sweet syntax.
> 
> The issue is what should _lock(InstX8632Lockable) do?  I could make
> InstX8632Lockable have a "setLocked()" method, and toggle w/ _lock().
> 
> What LLVM does is make LOCK a completely separate instruction. So it would be
> like Context.insert(LOCK), but then the syntax would be more like _lock();
> _cmpxchg(...);.
> 
> I didn't like that because it separates the prefix from the instruction, which
> means that say, when you want to know if the instruction is locked (for
whatever
> reason), you have to look backward a bit.


As we discussed, I'll leave this alone for now. The _lock(_inst()) syntax also
requires modifying the relevant _inst() to return the created inst, and
currently the _inst methods are all void.

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

https://codereview.chromium.org/362463002/diff/350013/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nacl-atomic-intrinsics.ll:233: ; CHECK: lock cmpxchg8b
qword ptr [e{{.[^x]}}]
On 2014/07/08 04:50:19, stichnot wrote:
> For the cmpxchg8b tests, it would be good to check for "push ebx" in the
prolog.
>  I think that something would have to go very wrong to trigger that failure,
but
> still...

Done.

[Jim Stichnoth, 2014-07-09 18:14:28]

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h
File src/IceInstX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:332: static InstX8632Unaryop *create(Cfg *Func, Variable
*SrcDest) {
On 2014/07/09 17:07:55, jvoung wrote:
> On 2014/07/08 18:14:07, jvoung wrote:
> > On 2014/07/08 04:50:19, stichnot wrote:
> > > An instruction like this is valid x86:
> > >   neg [eax+4*ecx]
> > > but this implementation of UnaryOp only allows these forms:
> > >   neg eax
> > >   neg [esp+16]
> > > since SrcDest is a Variable.  That's probably sufficient for our needs,
but
> > > ought to be documented.
> > > 
> > > Another possibility might be to declare SrcDest as Operand*, and set
> > > Dest=llvm::dyn_cast<Variable>(SrcDest), but I don't know if that would
> create
> > > any liveness problems.  (BTW, I see that you've done exactly that with
> > cmpxchg.)
> > 
> > Okay, I think I will try making it a more generic Operand.
> 
> Done.
> 
> Not sure about liveness either, but Dest would only be null if it's not a
> Variable, and then that would mean memory. In that case, it's like a store,
> where Dest is also null?

Yes, that's also what I'd expect.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:557: // possibly changed.
On 2014/07/09 17:07:55, jvoung wrote:
> On 2014/07/08 18:14:07, jvoung wrote:
> > On 2014/07/08 04:50:19, stichnot wrote:
> > > Would it be simpler to swap the roles of Eax and DestOrAddr with respect
to
> > the
> > > Dest operand?  That way, the instruction would always have a non-NULL
Dest,
> > and
> > > the lowering code could conditionally create the FakeDef if DestOrAddr is
a
> > > Variable.
> > 
> > I think this is similar to Xchg. See below?
> 
> Okay, this is slightly different from Xchg, in that it has an explicit Locked
> parameter.
> 
> As we discussed, if DestOrAddr is a memory operand, and we don't want it to be
> DCE'ed when Eax is unused. Locked=true helps because it marks the instruction
as
> HasSideEffects. It doesn't help when Locked=false (the caller has the option
to
> make that mistake)... I'm not sure sure how likely that is. Interestingly
> enough, in contrast, Xchg seems more useful to use not-Locked, though Locking
is
> automatic there (can be used for 16-bit bswap w/ two register operands)...
> 

After more thought, I would go simple and just disable DCE of xchg and cmpxchg
instructions always.  Using a NULL dest is an "approved" way of doing this, and
is minimally disruptive to the current patch since the emit/dump code doesn't
reference Dest.

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:581: // <m64> must be a memory operand.
On 2014/07/09 17:07:55, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > Document the user's responsibility to add appropriate FakeDefs.
> > 
> > BTW, in the style of some other 64-bit integer instructions, you could also
> > define eax to be the Dest variable, and then only have to create a FakeDef
for
> > edx.
> 
> Documented.
> 
> The other 64-bit integer instruction touching eax would be mul?

I was thinking of the 64-bit ret, but there may be others.

> 
> I feel that this a bit different from that, and this is more like a store
> instead.
> 
> For mul, what's modified is registers (edx:eax), so it makes sense to just put
> Eax in the Dest so that you don't need two FakeDefs and you have some Dest'ed
> variable to chain the edx FakeDef against. However, cmpxchg8b, like cmpxchg,
> will modify memory as well and the choice of Locked=true/false is again up to
> the user.
> 
> Maybe for cmpxchg8b it will always "HaveSideEffects", because one of the dests
> is always memory (in contrast to cmpxchg, where there is a choice).

As I commented elsewhere, I changed my mind and think that these xchg
instructions should probably have NULL dest, so never mind my earlier
suggestion. :)

https://codereview.chromium.org/362463002/diff/350013/src/IceInstX8632.h#newc...
src/IceInstX8632.h:950: static InstX8632Xchg *create(Cfg *Func, Operand *Dest,
Variable *Source) {
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > Can we structure this so that Dest is the Variable and Source is the
Operand?
> 
> I think the motivation for this was to make it like "Store" in the sense that
if
> Dest is not a variable, then Dest is NULL and it cannot be DCE'ed when the
Dest
> is actually a dead variable (it shouldn't be because Source might be memory
and
> we don't know if that is a dead store).
> 
> If we flip Dest and Source, is there another way to prevent it from DCE'ed?
Just
> mark the instruction as HasSideEffects when the source is memory?

At least for now, I would just be conservative and mark HasSideEffects whenever
the instruction is locked.  The main purpose of HasSideEffects is to suppress
DCE.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2169: // choosing eax for both. Work around this
for now, but forcing Addr
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > I'd like to understand more about this.  It sounds like something is going
> wrong
> > with register allocation (maybe the Om1 hack that passes for register
> > allocation?) or with lowering.
> > 
> > Also, "but" --> "by" ?
> > 
> > Same comments on the same comment below.
> 
> The problem was actually with O2 register allocation.
> 
> I think what happens is the following:
> 
> - Reg alloc notices that LegalPtr has an overlapping liverange w/ T_eax. That
> part is good.
> - However, LegalPtr has a preferred register, namely, whatever register was
> allocated for the earlier "Ptr" variable.
> - "Ptr" *does not* have a live range that overlaps T_eax (it ends when we
> convert Ptr to LegalPtr).
> - Ptr gets allocated to Eax.
> - Since LegalPtr prefers whatever reg Ptr gets, then it also gets Eax.
> - I don't think there is any additional checking for overlap w/ a pre-colored
> register, once there is a preferred register.

OK, then this is almost certainly a regalloc bug which I need to reproduce and
investigate.  In the meantime, maybe add a TODO to remove the Reg_edi stuff
after register allocation is fixed.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.h (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.h:180: void _cmpxchg(Operand *DestOrAddr, Variable
*Eax, Variable *Desired,
On 2014/07/08 18:14:07, jvoung wrote:
> On 2014/07/08 04:50:19, stichnot wrote:
> > This is probably not worth doing now, but in the future it might be nice to
be
> > able to write stylized lowering code like:
> > 
> >   _lock(_cmpxchg(...));
> 
> Yes that would be pretty sweet syntax.
> 
> The issue is what should _lock(InstX8632Lockable) do?  I could make
> InstX8632Lockable have a "setLocked()" method, and toggle w/ _lock().
> 
> What LLVM does is make LOCK a completely separate instruction. So it would be
> like Context.insert(LOCK), but then the syntax would be more like _lock();
> _cmpxchg(...);.
> 
> I didn't like that because it separates the prefix from the instruction, which
> means that say, when you want to know if the instruction is locked (for
whatever
> reason), you have to look backward a bit.

Yeah, I wouldn't like a separate lock instruction either, also because if you
want to delete an instruction, you also have to look back a bit to delete
prefixes as well.

I think we can revisit this once we have more than a small handful of cases
where we want to lower to a locked instruction.

[Jim Stichnoth, 2014-07-09 22:59:59]

So, this LGTM assuming JF is good with it.

For the record, the register allocation bug that requires hard-coding the edi
register is described in
https://code.google.com/p/nativeclient/issues/detail?id=3897 .

[jvoung (off chromium), 2014-07-10 23:14:50]

With https://code.google.com/p/nativeclient/issues/detail?id=3897 fixed
(thanks!), I removed the register allocation workarounds.

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/362463002/diff/350013/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:2169: // choosing eax for both. Work around this
for now, but forcing Addr
On 2014/07/09 18:14:28, stichnot wrote:
> On 2014/07/08 18:14:07, jvoung wrote:
> > On 2014/07/08 04:50:19, stichnot wrote:
> > > I'd like to understand more about this.  It sounds like something is going
> > wrong
> > > with register allocation (maybe the Om1 hack that passes for register
> > > allocation?) or with lowering.
> > > 
> > > Also, "but" --> "by" ?
> > > 
> > > Same comments on the same comment below.
> > 
> > The problem was actually with O2 register allocation.
> > 
> > I think what happens is the following:
> > 
> > - Reg alloc notices that LegalPtr has an overlapping liverange w/ T_eax.
That
> > part is good.
> > - However, LegalPtr has a preferred register, namely, whatever register was
> > allocated for the earlier "Ptr" variable.
> > - "Ptr" *does not* have a live range that overlaps T_eax (it ends when we
> > convert Ptr to LegalPtr).
> > - Ptr gets allocated to Eax.
> > - Since LegalPtr prefers whatever reg Ptr gets, then it also gets Eax.
> > - I don't think there is any additional checking for overlap w/ a
pre-colored
> > register, once there is a preferred register.
> 
> OK, then this is almost certainly a regalloc bug which I need to reproduce and
> investigate.  In the meantime, maybe add a TODO to remove the Reg_edi stuff
> after register allocation is fixed.

Done.

[JF, 2014-07-13 19:56:12]

I didn't check the lit tests, I assume Jim validated them :-)

The rest lgtm after a few small comments.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.h (right):

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic.h:1: #include "test_sync_atomic.def"
You're missing the license here and in other files.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
File crosstest/test_sync_atomic_main.cpp (right):

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:6: #include <stdint.h>
<cstdint>

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:96: << (8 * sizeof(Type)) << "("
s/8/CHAR_BIT/

Here and 2 other places below.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:103: << std::endl;
I'd just use '\n', since std::endl also leads to a flush. You probably only need
to flush at the end of the thread, and the test will run faster. Same for the
other 2 uses below.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:218: pthread_create(&t[m], NULL,
&threadWrapper<Type>,
Check the return code.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:224: pthread_join(t[m], NULL);
Ditto.

[jvoung (off chromium), 2014-07-14 16:20:31]

Thanks!

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
File crosstest/test_sync_atomic.h (right):

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic.h:1: #include "test_sync_atomic.def"
On 2014/07/13 19:56:12, JF wrote:
> You're missing the license here and in other files.

So far, none of the crosstest .def, .h, .cpp files had licenses, but I'll them
to this one.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
File crosstest/test_sync_atomic_main.cpp (right):

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:6: #include <stdint.h>
On 2014/07/13 19:56:12, JF wrote:
> <cstdint>

The tests are currently not compiling w/ -std= anything, and cstdint requires
c++0x or higher.

Since changing the -std=, will end up changing the other tests (I think there
were some -Wnarrowing issues), I'd like to leave this alone for now.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:96: << (8 * sizeof(Type)) << "("
On 2014/07/13 19:56:12, JF wrote:
> s/8/CHAR_BIT/
> 
> Here and 2 other places below.

Done.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:103: << std::endl;
On 2014/07/13 19:56:12, JF wrote:
> I'd just use '\n', since std::endl also leads to a flush. You probably only
need
> to flush at the end of the thread, and the test will run faster. Same for the
> other 2 uses below.

FWIW: this only prints when there is an error, so it shouldn't really affect the
test speed =)

In any case, these prints are also in the main thread, so I'll just convert them
to \n.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:218: pthread_create(&t[m], NULL,
&threadWrapper<Type>,
On 2014/07/13 19:56:12, JF wrote:
> Check the return code.

Done.

https://codereview.chromium.org/362463002/diff/490001/crosstest/test_sync_ato...
crosstest/test_sync_atomic_main.cpp:224: pthread_join(t[m], NULL);
On 2014/07/13 19:56:12, JF wrote:
> Ditto.

Done.

[jvoung (off chromium), 2014-07-14 17:32:47]

Committed patchset #19 manually as ra3a01a2 (presubmit successful).