Add BL (immediate) and BLX (register) to ARM assembler.

Add BL (immediate) and BLX (register) to ARM assembler.

Adds BL and BLX to ARM integrated assembler.

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

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

[Karl, 2015-11-17 23:57:16]

Description was changed from

==========
Add BL (immediate) and BLX (register) to ARM assembler.

Adds BL and BLX to ARM integrated assembler.

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

to

==========
Add BL (immediate) and BLX (register) to ARM assembler.

Adds BL and BLX to ARM integrated assembler.

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

[Karl, 2015-11-17 23:57:16]

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

[John, 2015-11-18 01:04:17]

Few nits, otherwise lgtm.

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:344: new (allocate<BlRelocatableFixup>())
BlRelocatableFixup();
The usual idiom is to have a static create method in BlRelocatableFixup that
takes a Cfg, and returns a newly allocated object.

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:54: size_t emit(GlobalContext *Ctx, const Assembler
&Asm) const final;
Optional: make the whole class final?

https://codereview.chromium.org/1452293003/diff/1/tests_lit/assembler/arm32/p...
File tests_lit/assembler/arm32/push-pop.ll (right):

https://codereview.chromium.org/1452293003/diff/1/tests_lit/assembler/arm32/p...
tests_lit/assembler/arm32/push-pop.ll:137: ; IASM-NEXT:    bl      DoSomething	@
.word ebfffffe
Replace tab with whitespace?

[Karl, 2015-11-18 16:19:32]

Committed patchset #2 (id:20001) manually as
174531ec83bf9d51b2dc669b3f73f1fb637fbcce (presubmit successful).

[Karl, 2015-11-18 16:19:37]

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.cpp
File src/IceAssemblerARM32.cpp (right):

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.cpp#n...
src/IceAssemblerARM32.cpp:344: new (allocate<BlRelocatableFixup>())
BlRelocatableFixup();
On 2015/11/18 01:04:17, John wrote:
> The usual idiom is to have a static create method in BlRelocatableFixup that
> takes a Cfg, and returns a newly allocated object.

I was following the pattern for (all) other fixups. The create methods are in
the assembler class, not the fixup class.

Also, not sure where Cfg would come from since the fixups are generated in the
assembler, and assembler is designed to NOT require a Cfg. Note that the
assembler has it's own allocator, and that is the allocator we are using.

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.h
File src/IceAssemblerARM32.h (right):

https://codereview.chromium.org/1452293003/diff/1/src/IceAssemblerARM32.h#new...
src/IceAssemblerARM32.h:54: size_t emit(GlobalContext *Ctx, const Assembler
&Asm) const final;
On 2015/11/18 01:04:17, John wrote:
> Optional: make the whole class final?

Done.

https://codereview.chromium.org/1452293003/diff/1/tests_lit/assembler/arm32/p...
File tests_lit/assembler/arm32/push-pop.ll (right):

https://codereview.chromium.org/1452293003/diff/1/tests_lit/assembler/arm32/p...
tests_lit/assembler/arm32/push-pop.ll:137: ; IASM-NEXT:    bl      DoSomething	@
.word ebfffffe
On 2015/11/18 01:04:17, John wrote:
> Replace tab with whitespace?

Done.

[Jim Stichnoth, 2015-11-22 03:27:03]

otherwise necro lgtm.

Try to address comments in a future CL?

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

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:336: Str << "\tbl\t" << symbol(Ctx) << "\t@ .word "
At least on the x86 emit() side, we try to separate the \t from the opcode,
e.g.:

  Str << "\t" << "bl\t" << ...

This is so you can find uses of the opcode easier using "git grep -w bl".

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:737: int32_t Encoding = (static_cast<int32_t>(Cond) <<
kConditionShift) | B24 |
IValueT instead of int32_t, right?

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

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:116: BlRelocatableFixup *createBlFixup(const
ConstantRelocatable *Target);
I'd be happier if we reserved variable name "Target" for a TargetLowering*. 
Better might be BrTarget or BlTarget.

https://codereview.chromium.org/1452293003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/blx.ll (right):

https://codereview.chromium.org/1452293003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/blx.ll:1: ; Show that we know how to translate add.
add?

[Karl, 2015-11-30 16:54:35]

Fixed in CL https://codereview.chromium.org/1484023002.

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

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:336: Str << "\tbl\t" << symbol(Ctx) << "\t@ .word "
On 2015/11/22 03:27:03, stichnot wrote:
> At least on the x86 emit() side, we try to separate the \t from the opcode,
> e.g.:
> 
>   Str << "\t" << "bl\t" << ...
> 
> This is so you can find uses of the opcode easier using "git grep -w bl".

Done.

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.c...
src/IceAssemblerARM32.cpp:737: int32_t Encoding = (static_cast<int32_t>(Cond) <<
kConditionShift) | B24 |
On 2015/11/22 03:27:03, stichnot wrote:
> IValueT instead of int32_t, right?

Replaced with encodeCondition(Cond).

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

https://codereview.chromium.org/1452293003/diff/20001/src/IceAssemblerARM32.h...
src/IceAssemblerARM32.h:116: BlRelocatableFixup *createBlFixup(const
ConstantRelocatable *Target);
On 2015/11/22 03:27:03, stichnot wrote:
> I'd be happier if we reserved variable name "Target" for a TargetLowering*. 
> Better might be BrTarget or BlTarget.

Done.

https://codereview.chromium.org/1452293003/diff/20001/tests_lit/assembler/arm...
File tests_lit/assembler/arm32/blx.ll (right):

https://codereview.chromium.org/1452293003/diff/20001/tests_lit/assembler/arm...
tests_lit/assembler/arm32/blx.ll:1: ; Show that we know how to translate add.
On 2015/11/22 03:27:03, stichnot wrote:
> add?

Replaced with blx.