Add NOP to ARM IR lowering.

Add NOP to ARM IR lowering.

This change allows pnacl-sz to randomly insert NOPs into the generated
code, as is already done with X86.

BUG=None
R=eholk@chromium.org, stichnot@chromium.org

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

[Karl, 2016-02-05 21:53:03]

Description was changed from

==========
Add NOP to ARM IR lowering.

This change allows pnacl-sz to randomly insert NOPs into the generated
code, as is already done with X86.

BUG=None
==========

to

==========
Add NOP to ARM IR lowering.

This change allows pnacl-sz to randomly insert NOPs into the generated
code, as is already done with X86.

BUG=None
==========

[Karl, 2016-02-05 21:53:03]

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

[Eric Holk, 2016-02-08 17:29:43]

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred {
Does it make sense to have Nop derive from a predicable instruction?

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nop-insertion.ll (right):

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nop-insertion.ll:149: ; ARM110P2-NEXT:       
.bundle_unlock
Out of curiosity, what are these .bundle_unlock and .bundle_lock things for?

[Karl, 2016-02-08 17:55:55]

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred {
On 2016/02/08 17:29:43, Eric Holk wrote:
> Does it make sense to have Nop derive from a predicable instruction?

The ARM instruction allows predicates, and so like other instructions, I made it
a subclass of this.

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nop-insertion.ll (right):

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nop-insertion.ll:149: ; ARM110P2-NEXT:       
.bundle_unlock
On 2016/02/08 17:29:43, Eric Holk wrote:
> Out of curiosity, what are these .bundle_unlock and .bundle_lock things for?

These directives are the way we make sure that sandboxing for NaCl works. In
NaCl, instructions are in 16 byte bundles, Branches can only be to the beginning
of such bundles. A bundle_lock/bundle_unlock forces the assembler to put all
instructions between the two in a bundle.

[Eric Holk, 2016-02-08 17:58:15]

lgtm

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h
File src/IceInstARM32.h (right):

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.h#newc...
src/IceInstARM32.h:1438: class InstARM32Nop final : public InstARM32Pred {
On 2016/02/08 17:55:55, Karl wrote:
> On 2016/02/08 17:29:43, Eric Holk wrote:
> > Does it make sense to have Nop derive from a predicable instruction?
> 
> The ARM instruction allows predicates, and so like other instructions, I made
it
> a subclass of this.

That seems reasonable.

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nop-insertion.ll (right):

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nop-insertion.ll:149: ; ARM110P2-NEXT:       
.bundle_unlock
On 2016/02/08 17:55:55, Karl wrote:
> On 2016/02/08 17:29:43, Eric Holk wrote:
> > Out of curiosity, what are these .bundle_unlock and .bundle_lock things for?
> 
> These directives are the way we make sure that sandboxing for NaCl works. In
> NaCl, instructions are in 16 byte bundles, Branches can only be to the
beginning
> of such bundles. A bundle_lock/bundle_unlock forces the assembler to put all
> instructions between the two in a bundle.

Thanks!

[Jim Stichnoth, 2016-02-08 18:17:51]

lgtm

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1097: InstARM32Nop::InstARM32Nop(Cfg *Func)
Should nop (and dmb above) be inserted into the list more alphabetically?

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:2321: Func->getContext()->getStrDump() << "\t"
dump() doesn't usually prepend a tab character.

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nop-insertion.ll (right):

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nop-insertion.ll:159: ; ARM110P2-NEXT:        # [sp,
#32] = def.pseudo
All these def.pseudo lines seem to have a trailing space character.

(I guess FakeDef's emit routine ought to be fixed...)

[Karl, 2016-02-09 21:09:26]

Description was changed from

==========
Add NOP to ARM IR lowering.

This change allows pnacl-sz to randomly insert NOPs into the generated
code, as is already done with X86.

BUG=None
==========

to

==========
Add NOP to ARM IR lowering.

This change allows pnacl-sz to randomly insert NOPs into the generated
code, as is already done with X86.

BUG=None
R=eholk@chromium.org, stichnot@chromium.org

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

[Karl, 2016-02-09 21:09:27]

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

[Karl, 2016-02-09 21:10:02]

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp
File src/IceInstARM32.cpp (right):

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:1097: InstARM32Nop::InstARM32Nop(Cfg *Func)
On 2016/02/08 18:17:50, stichnot wrote:
> Should nop (and dmb above) be inserted into the list more alphabetically?

Really? Where do you see sorting? For the most part, I have no idea how this
file was sorted. However, I will at least sort this short list of constructors.

https://codereview.chromium.org/1670413002/diff/20001/src/IceInstARM32.cpp#ne...
src/IceInstARM32.cpp:2321: Func->getContext()->getStrDump() << "\t"
On 2016/02/08 18:17:50, stichnot wrote:
> dump() doesn't usually prepend a tab character.

Done.

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/nop-insertion.ll (right):

https://codereview.chromium.org/1670413002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/nop-insertion.ll:159: ; ARM110P2-NEXT:        # [sp,
#32] = def.pseudo
On 2016/02/08 18:17:50, stichnot wrote:
> All these def.pseudo lines seem to have a trailing space character.
> 
> (I guess FakeDef's emit routine ought to be fixed...)

Removed spaces. Fixing the dump() method for FakeDef is harder. It calls
emitSources() which doesn't add a space. Will look into fixing this in another
CL.

[Jim Stichnoth, 2016-02-09 21:22:40]

> On 2016/02/08 18:17:50, stichnot wrote:
> > All these def.pseudo lines seem to have a trailing space character.
> > 
> > (I guess FakeDef's emit routine ought to be fixed...)
> 
> Removed spaces. Fixing the dump() method for FakeDef is harder. It calls
> emitSources() which doesn't add a space. Will look into fixing this in another
> CL.

FYI, my pending CL takes care of this, so need to do it.