Add various forms of LDREX/STREX to ARM integrated assembler.

Add various forms of LDREX/STREX to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-12-10 18:49:51]

Description was changed from

==========
Add various forms of LDREX/STREX to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add various forms of LDREX/STREX to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-12-10 18:49:51]

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

[John, 2015-12-11 16:59:09]

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1249: switch (typeWidthInBytesLog2(Ty)) {
Why did you switch on the type width? can you switch (Ty) instead?

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/ldrex-strex.ll (right):

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/ldrex-strex.ll:220: %v = call i16
@llvm.nacl.atomic.rmw.i16(i32 1, i16* %ptr.asptr, i16 %a.arg_trunc, i32 6)
80-col? same for the others.

[Jim Stichnoth, 2015-12-11 17:24:57]

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:196: IValueT getVarRegNum(const Variable *Var) {
This function name should be changed, to reflect that fact that it is returning
an encoded register number.

Maybe getEncodedReg or getEncodedRegNum ?

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:215: // case DefaultOpEncoding:
This style of comment made me think you had some stray commented code lying
around.  What do you think of this style:

// *** DefaultOpEncoding ***
// Value=...
//
// **** OpEncoding3 ***
// Value=...

or something along those lines?

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1264: // emitMemExOp(Cond, MemExOpcode, Rd, Rt,
OpAddress, TInfo, InstName);
Remove this comment?

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1611: return;
Remove this explicit return.

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/ldrex-strex.ll (right):

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/ldrex-strex.ll:220: %v = call i16
@llvm.nacl.atomic.rmw.i16(i32 1, i16* %ptr.asptr, i16 %a.arg_trunc, i32 6)
On 2015/12/11 16:59:09, John wrote:
> 80-col? same for the others.

We generally haven't enforced 80-col on .ll tests.  Is it even possible to use \
continuation?  If so, I guess go for it, but it also deserves a separate cleanup
CL (perhaps by someone who feels like procrastinating...).

[Karl, 2015-12-11 18:05:08]

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:196: IValueT getVarRegNum(const Variable *Var) {
On 2015/12/11 17:24:57, stichnot wrote:
> This function name should be changed, to reflect that fact that it is
returning
> an encoded register number.
> 
> Maybe getEncodedReg or getEncodedRegNum ?

Changed to getEncodedGPRegNum.

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:215: // case DefaultOpEncoding:
On 2015/12/11 17:24:57, stichnot wrote:
> This style of comment made me think you had some stray commented code lying
> around.  What do you think of this style:
> 
> // *** DefaultOpEncoding ***
> // Value=...
> //
> // **** OpEncoding3 ***
> // Value=...
> 
> or something along those lines?

Ok. Done.

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1249: switch (typeWidthInBytesLog2(Ty)) {
On 2015/12/11 16:59:09, John wrote:
> Why did you switch on the type width? can you switch (Ty) instead?

I did it this way for the following reasons:

1) I wanted only one source of type width (for each type), so that if it
changes, no other code needs fixing.

2) Using the log2 form, there is a simple contiguous switch selection (allowing
possible optimization of the switch).

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1264: // emitMemExOp(Cond, MemExOpcode, Rd, Rt,
OpAddress, TInfo, InstName);
On 2015/12/11 17:24:57, stichnot wrote:
> Remove this comment?

Really? I like useless comments. Removing...

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1611: return;
On 2015/12/11 17:24:57, stichnot wrote:
> Remove this explicit return.

Done.

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/ldrex-strex.ll (right):

https://codereview.chromium.org/1516863003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/ldrex-strex.ll:220: %v = call i16
@llvm.nacl.atomic.rmw.i16(i32 1, i16* %ptr.asptr, i16 %a.arg_trunc, i32 6)
On 2015/12/11 17:24:57, stichnot wrote:
> On 2015/12/11 16:59:09, John wrote:
> > 80-col? same for the others.
> 
> We generally haven't enforced 80-col on .ll tests.  Is it even possible to use
\
> continuation?  If so, I guess go for it, but it also deserves a separate
cleanup
> CL (perhaps by someone who feels like procrastinating...).

LLVM IR is free-style, not line based. Fixing by wrapping lines.

[John, 2015-12-11 18:10:46]

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1249: switch (typeWidthInBytesLog2(Ty)) {
On 2015/12/11 18:05:08, Karl wrote:
> On 2015/12/11 16:59:09, John wrote:
> > Why did you switch on the type width? can you switch (Ty) instead?
> 
> I did it this way for the following reasons:
> 
> 1) I wanted only one source of type width (for each type), so that if it
> changes, no other code needs fixing.
> 
> 2) Using the log2 form, there is a simple contiguous switch selection
(allowing
> possible optimization of the switch).

IceType_i[8|16|32|64] are declared sequentially in the enum, so the compiler
should be able to optimize this.

Please, switch on Ty. It will make the code explicit (i.e., ldrex/strex operate
on integer types.) anything other than i8, i16, i32, or i64 here is a mistake
and should be reported.

[Jim Stichnoth, 2015-12-11 20:17:25]

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1202: // using two (32-bit) load instructions. Note:
Intenionally drop to default
Intentionally

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1553: // using two (32-bit) store instructions.  Note:
Intenionally drop to
Intentionally

[Karl, 2015-12-11 21:00:12]

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1249: switch (typeWidthInBytesLog2(Ty)) {
On 2015/12/11 18:10:46, John wrote:
> On 2015/12/11 18:05:08, Karl wrote:
> > On 2015/12/11 16:59:09, John wrote:
> > > Why did you switch on the type width? can you switch (Ty) instead?
> > 
> > I did it this way for the following reasons:
> > 
> > 1) I wanted only one source of type width (for each type), so that if it
> > changes, no other code needs fixing.
> > 
> > 2) Using the log2 form, there is a simple contiguous switch selection
> (allowing
> > possible optimization of the switch).
> 
> IceType_i[8|16|32|64] are declared sequentially in the enum, so the compiler
> should be able to optimize this.
> 
> Please, switch on Ty. It will make the code explicit (i.e., ldrex/strex
operate
> on integer types.) anything other than i8, i16, i32, or i64 here is a mistake
> and should be reported.

OK, I'll change it, but you are slightly incorrect. type i1 uses the "b" form
also.

We should also be consistent, so I'm fixing the other cases where
typeWidthInBits is used.

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1202: // using two (32-bit) load instructions. Note:
Intenionally drop to default
On 2015/12/11 20:17:25, stichnot wrote:
> Intentionally

Done.

https://codereview.chromium.org/1516863003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:1553: // using two (32-bit) store instructions.  Note:
Intenionally drop to
On 2015/12/11 20:17:25, stichnot wrote:
> Intentionally

Done.

[Jim Stichnoth, 2015-12-11 21:25:52]

lgtm

[Karl, 2015-12-17 16:06:03]

Description was changed from

==========
Add various forms of LDREX/STREX to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
==========

to

==========
Add various forms of LDREX/STREX to ARM integrated assembler.

BUG= https://bugs.chromium.org/p/nativeclient/issues/detail?id=4334
R=stichnot@chromium.org

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

[Karl, 2015-12-17 16:06:05]

Committed patchset #4 (id:60001) manually as
4175d45e14cf1eb0af11e0458a873e601dc395de (presubmit successful).