Thumb2 Backend: 16-bit instruction encoding helper methods

Thumb2 Backend: 16-bit instruction encoding helper methods

This patch defines helper methods for the encoding of 16-bit Thumb2 instructions.

This is the 1st patch for the Thumb2 Backend.
Next patch: https://codereview.chromium.org/24182004/

BUG=none
TEST=none

[rmcilroy_google, 2013-09-17 17:28:15]

Hi Rejeev,

Thanks for sending these changes! I'm excited to see what impact using Thumb2
can have on generated code size / performance.

I have a couple of overall structural comments, please let me know what you
think.

Also, I think your checkout might be a bit messed up. Your "Base URL" field is
set to HEAD^, when it should be set to something like
https://v8.googlecode.com/svn/branches/bleeding_edge.  Did you follow the
instructions at https://code.google.com/p/v8/wiki/UsingGit?

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h#n...
src/arm/assembler-arm-inl.h:389: void Assembler::emit16(Instr16 x, bool
check_buffer) {
Why do you need the check_buffer parameter?  There don't seem to be any uses of
it in this CL.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h#n...
src/arm/assembler-arm-inl.h:394: pc_ += 2;
Please make this a constant (e.g., kThumb2InstrSize)

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm.h
File src/arm/assembler-arm.h (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:1367: Instr16 thumb16_instr_at(int pos) {
thumb2_instr16_at.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm.h#newco...
src/arm/assembler-arm.h:1552: Instr16 thumb16_mode1(ThumbMode1Opcode16 op);  //
A6.2.1 -> mode1
ditto with thumb16->thumb2 here and below. Also, are these really necessary in
the public header? Maybe they could go in assembler-arm-int.h, or if you end up
splitting thumb into a seperate Assembler class (see comment in
assembler-thumb16.cc) then in assembler-thumb2-int.h.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc
File src/arm/assembler-thumb16.cc (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc#...
src/arm/assembler-thumb16.cc:1: // Copyright 2012 the V8 project authors. All
rights reserved.
2013

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc#...
src/arm/assembler-thumb16.cc:36: namespace internal {
I'm not sure whether this should be a separate file from assembler-arm.cc given
that it adds functionality to the Assembler class. However, I do think we should
partition the thumb2 and arm ISA assembler code.  What would be the implications
of separating this out into a separate AssemblerThumb2 class which extends
AssemblerBase?  I'm not sure how this would work with later patches where you
actually start using thumb instructions in the macro-assembler/codegen, but I
think something like this might be cleaner in the end.  One way to do this might
be to change MacroAssembler to inherit from an abstract Assembler (rather than
the concrete Arm ISA assembler as now), and delegate the actual functionality to
either a AssemblerArm or AssemblerThumb2 depending on the mode it is in. Any
thoughts on this from other folks?

In any case, if it remains a seperate file I think it should be renamed
assembler-thumb2.cc (I see in later patches you create an assembler-thumb32.cc
file, but I think this is unecessary and all thumb code, whether for 32 or 16
bit instructions should be together).

https://codereview.chromium.org/23560010/diff/1/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/constants-arm.h#newco...
src/arm/constants-arm.h:145: typedef int16_t Instr16;
Add a comment, as per the comment for Instr above.  Also, when you add 32bit
thumb instructions, it would probably be good to seperate them from Instr above,
so maybe name this Thumb2Instr16 and have a Thumb2Instr32 for 32 bit thumb
instructions?

https://codereview.chromium.org/23560010/diff/1/src/arm/constants-arm.h#newco...
src/arm/constants-arm.h:170: enum ThumbMode1Opcode16 {
I'm not keen on having these multiple enums for different "modes" of Thumb
instructions. As far as I can see, the regular arm Opcode enum is only used for
disassembly, not code generation.  Can you get away with merging these emums and
merging multiple different types of the same instruction (e.g., ADD_REG_1,
ADD_IMM_1, ADD_IMM_2, ADD_REG_2) 
into a single ADD opcode, with methods on an Instr16 to disambiguate the source
and destination location types?

[rkrithiv, 2013-09-27 21:40:49]

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h#n...
src/arm/assembler-arm-inl.h:389: void Assembler::emit16(Instr16 x, bool
check_buffer) {
This is used in later patches.

On 2013/09/17 17:28:16, rmcilroy_google wrote:
> Why do you need the check_buffer parameter?  There don't seem to be any uses
of
> it in this CL.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc
File src/arm/assembler-thumb16.cc (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc#...
src/arm/assembler-thumb16.cc:1: // Copyright 2012 the V8 project authors. All
rights reserved.
On 2013/09/17 17:28:16, rmcilroy_google wrote:
> 2013

Done.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc#...
src/arm/assembler-thumb16.cc:36: namespace internal {
On 2013/09/17 17:28:16, rmcilroy_google wrote:
> I'm not sure whether this should be a separate file from assembler-arm.cc
given
> that it adds functionality to the Assembler class. However, I do think we
should
> partition the thumb2 and arm ISA assembler code.  What would be the
implications
> of separating this out into a separate AssemblerThumb2 class which extends
> AssemblerBase?  I'm not sure how this would work with later patches where you
> actually start using thumb instructions in the macro-assembler/codegen, but I
> think something like this might be cleaner in the end.  
I agree that having separate AssemblerARM and AssemblerThumb2 would make the
implementation a lot cleaner. This should not be too difficult to get working
with the changes to macro-assembler/codegen for Thumb2.

> One way to do this might
> be to change MacroAssembler to inherit from an abstract Assembler (rather than
> the concrete Arm ISA assembler as now), and delegate the actual functionality
to
> either a AssemblerArm or AssemblerThumb2 depending on the mode it is in. Any
> thoughts on this from other folks?
>
Could you elaborate on this idea? One thing to remember is that some Assembler
encoding methods (for VFP/SIMD instructions) are common betweeen ARM and Thumb2
modes. We could have a common Assembler class (which would inherit from
AssemblerBase) define the common encoding methods, then have the AssemblerARM
and AssemblerThumb2 classes inherit from the common Assembler class and define
the other encoding methods. And finally we could have a templatized
MacroAssembler class that inherits from either the AssemblerARM or
AssemblerThumb2 based on the mode. There are multiple ways in which we could
separate the two Assemblers, please let me know what design you think would work
best for this.

 
> In any case, if it remains a seperate file I think it should be renamed
> assembler-thumb2.cc (I see in later patches you create an assembler-thumb32.cc
> file, but I think this is unecessary and all thumb code, whether for 32 or 16
> bit instructions should be together).

Sure if we decide to keep a single Assembler for ARM and Thumb2, I can refactor
the code in this manner.

[rmcilroy, 2013-10-01 10:26:04]

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h
File src/arm/assembler-arm-inl.h (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-arm-inl.h#n...
src/arm/assembler-arm-inl.h:389: void Assembler::emit16(Instr16 x, bool
check_buffer) {
On 2013/09/27 21:40:49, rkrithiv wrote:
> This is used in later patches.
> 
> On 2013/09/17 17:28:16, rmcilroy_google wrote:
> > Why do you need the check_buffer parameter?  There don't seem to be any uses
> of
> > it in this CL.
> 

Please just do always do the CheckBuffer() here and re-introduce the boolean
argument in the CL when it is needed.

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc
File src/arm/assembler-thumb16.cc (right):

https://codereview.chromium.org/23560010/diff/1/src/arm/assembler-thumb16.cc#...
src/arm/assembler-thumb16.cc:36: namespace internal {
> > One way to do this might
> > be to change MacroAssembler to inherit from an abstract Assembler (rather
than
> > the concrete Arm ISA assembler as now), and delegate the actual
functionality
> to
> > either a AssemblerArm or AssemblerThumb2 depending on the mode it is in. Any
> > thoughts on this from other folks?
> >
> Could you elaborate on this idea? One thing to remember is that some Assembler
> encoding methods (for VFP/SIMD instructions) are common betweeen ARM and
Thumb2
> modes. We could have a common Assembler class (which would inherit from
> AssemblerBase) define the common encoding methods, then have the AssemblerARM
> and AssemblerThumb2 classes inherit from the common Assembler class and define
> the other encoding methods. And finally we could have a templatized
> MacroAssembler class that inherits from either the AssemblerARM or
> AssemblerThumb2 based on the mode. There are multiple ways in which we could
> separate the two Assemblers, please let me know what design you think would
work
> best for this.

My thoughts were along the same lines as yours.  Something like a
AssemblerCommon (with the common VFP/SIMD instructions, and abstract / pure
virtual methods for the common instructions). AssemblerArm and AssemblerThumb2
would extend this common class, and add their implementations of these
instructions. I wasn't thinking of templatizing MacroAssembler, I was simply
thinking the the MacroAssembler class would no longer extend any Assembler
class, but would simply create either an AssemblerArm or AssemblerThumb2 object,
and delegate all instruction generation methods to this object.  I'd like some
of the other reviewers to comment on this though, since there is a potential
performance cost to the extra level of indirection, and it would also involve
creating a bunch of methods in macroassembler.cc for the non-macro assembler
methods which simply delegate the call through to the actual underlying
assembler.

Daniel, Ulan, Benedikt, do you guys have any opinion on this?