Handle branch relative to pc in ARM integrated assembler.

Handle branch relative to pc in ARM integrated assembler.

Adds an explicit branch instruction (near form only), which allows
branching from the current pc up to 2**26 bytes (in either direction).
For now, this near restriction (within a function) doesn't appear to
be a bad restriction, and only near jumps have been implemented.

Also fixes notationally the concepts of the following types:

InstValueType : The 32-bit encoding of an instruction value.
InstOffsetType : Offset (+/-) used within an instruction.

BUG= https://code.google.com/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=137e62bd843313d4b8697621989e0e6f5c2e7f81

[Karl, 2015-10-23 21:00:32]

Description was changed from

==========
Handle branch relative to pc in ARM integrated assembler.

Adds an explicit branch instruction (near form only), which allows
branching from the current pc up to 2**26 bytes (in either direction).
For now, this near restriction (within a function) doesn't appear to
be a bad restriction, and only near jumps have been implemented.

Also fixes notationally the concepts of the following types:

InstValueType : The 32-bit encoding of an instruction value.
InstOffsetType : Offset (+/-) used within an instruction.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

to

==========
Handle branch relative to pc in ARM integrated assembler.

Adds an explicit branch instruction (near form only), which allows
branching from the current pc up to 2**26 bytes (in either direction).
For now, this near restriction (within a function) doesn't appear to
be a bad restriction, and only near jumps have been implemented.

Also fixes notationally the concepts of the following types:

InstValueType : The 32-bit encoding of an instruction value.
InstOffsetType : Offset (+/-) used within an instruction.

BUG= https://code.google.com/p/nativeclient/issues/detail?id=4334
==========

[Karl, 2015-10-23 21:00:32]

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

[Jim Stichnoth, 2015-10-23 21:32:08]

https://codereview.chromium.org/1418313003/diff/20001/src/DartARM32/assembler...
File src/DartARM32/assembler_arm.h (right):

https://codereview.chromium.org/1418313003/diff/20001/src/DartARM32/assembler...
src/DartARM32/assembler_arm.h:1126: #if 0
Have you considered joining this and the above into a combined "#if 0"/"#endif"
region, instead of two adjacent regions?  This might make it easier in the
future to identify assembler routines that haven't been brought over but maybe
should.  Your call.

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

https://codereview.chromium.org/1418313003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:254: // TODO(kschimpf): Decide if we need to handle
far jumps (ignoring for now).
For this TODO and the related one below, this is a correctness issue and as
such, I think you need to at least verify with an assert for the time in the
future when this unexpectedly triggers.

https://codereview.chromium.org/1418313003/diff/20001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1418313003/diff/20001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:42: using InstValueType = uint32_t;
You might consider shortening these to InstValueT and InstOffsetT, analogous to
Ice::SizeT.  Even shorter possibilities might be IValueT/IOffsetT or
ValueT/OffsetT.

https://codereview.chromium.org/1418313003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/branch-mult-fwd.ll (right):

https://codereview.chromium.org/1418313003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/branch-mult-fwd.ll:35: ; IASM-LABLE:mult_fwd_branches:
LABLE ?

[Karl, 2015-10-26 18:32:44]

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/DartARM32/as...
File src/DartARM32/assembler_arm.h (right):

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/DartARM32/as...
src/DartARM32/assembler_arm.h:1126: #if 0
On 2015/10/23 21:32:07, stichnot wrote:
> Have you considered joining this and the above into a combined "#if
0"/"#endif"
> region, instead of two adjacent regions?  This might make it easier in the
> future to identify assembler routines that haven't been brought over but maybe
> should.  Your call.

Sounds reasonable, since it make the file easier to read. Fixed all such cases
in the file (and did the same to assembler_arm.cc).

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.cpp (right):

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.cpp:254: // TODO(kschimpf): Decide if we need to handle
far jumps (ignoring for now).
On 2015/10/23 21:32:07, stichnot wrote:
> For this TODO and the related one below, this is a correctness issue and as
> such, I think you need to at least verify with an assert for the time in the
> future when this unexpectedly triggers.

Added function canEncodeBranchOffset to do check. Cleaned up comments to match.

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1418313003/diff/20001/src/IceAssembler...
src/IceAssemblerARM32.h:42: using InstValueType = uint32_t;
On 2015/10/23 21:32:07, stichnot wrote:
> You might consider shortening these to InstValueT and InstOffsetT, analogous
to
> Ice::SizeT.  Even shorter possibilities might be IValueT/IOffsetT or
> ValueT/OffsetT.

ValueT appeared too easy to confuse with other possibilities. Used IValueT and
IOffsetT.

https://chromiumcodereview.appspot.com/1418313003/diff/20001/tests_lit/assemb...
File tests_lit/assembler/arm32/branch-mult-fwd.ll (right):

https://chromiumcodereview.appspot.com/1418313003/diff/20001/tests_lit/assemb...
tests_lit/assembler/arm32/branch-mult-fwd.ll:35: ; IASM-LABLE:mult_fwd_branches:
On 2015/10/23 21:32:08, stichnot wrote:
> LABLE ?

Done.

[Jim Stichnoth, 2015-10-26 22:59:08]

otherwise lgtm

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

https://codereview.chromium.org/1418313003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:91: inline IValueT encodeBool(bool b) { return b ? 1 :
0; }
Probably want B instead of b

https://codereview.chromium.org/1418313003/diff/40001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:259: bool isGoodOffset =
canEncodeBranchOffset(Offset);
IsGoodOffset

https://codereview.chromium.org/1418313003/diff/40001/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1418313003/diff/40001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:180: // Encodes The given Offset into the branch
instruction Inst.
s/The/the/

[Karl, 2015-10-27 14:28:14]

Committed patchset #4 (id:60001) manually as
137e62bd843313d4b8697621989e0e6f5c2e7f81 (presubmit successful).

[Karl, 2015-10-27 14:28:27]

https://chromiumcodereview.appspot.com/1418313003/diff/40001/src/IceAssembler...
File src/IceAssemblerARM32.cpp (right):

https://chromiumcodereview.appspot.com/1418313003/diff/40001/src/IceAssembler...
src/IceAssemblerARM32.cpp:91: inline IValueT encodeBool(bool b) { return b ? 1 :
0; }
On 2015/10/26 22:59:08, stichnot wrote:
> Probably want B instead of b

Done.

https://chromiumcodereview.appspot.com/1418313003/diff/40001/src/IceAssembler...
src/IceAssemblerARM32.cpp:259: bool isGoodOffset =
canEncodeBranchOffset(Offset);
On 2015/10/26 22:59:08, stichnot wrote:
> IsGoodOffset

Done.

https://chromiumcodereview.appspot.com/1418313003/diff/40001/src/IceAssembler...
File src/IceAssemblerARM32.h (right):

https://chromiumcodereview.appspot.com/1418313003/diff/40001/src/IceAssembler...
src/IceAssemblerARM32.h:180: // Encodes The given Offset into the branch
instruction Inst.
On 2015/10/26 22:59:08, stichnot wrote:
> s/The/the/

Done.