ARM: Merging constants in simulator and assembler header files and other clea...

ARM: Merging constants in simulator and assembler header files and other cleanup.

First stab at a general ARM cleanup patch. It merges ARM constants so that they can be used across simulator, assembler and disassembler, and tidies up some syntax and ambiguities.

BUG=none
TEST=none

Committed: http://code.google.com/p/v8/source/detail?r=6483

[Mads Ager (chromium), 2011-01-20 12:40:00]

First overall comment: no using directives. :)

I'll have a look at the actual contents soon.

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

http://codereview.chromium.org/6274009/diff/1/src/arm/assembler-arm.h#newcode43
src/arm/assembler-arm.h:43: #include "constants-arm.h"
Can we keep the include list alphabeticed?

http://codereview.chromium.org/6274009/diff/1/src/arm/assembler-arm.h#newcode47
src/arm/assembler-arm.h:47: using namespace assembler::arm;
We are not using 'using' directives. Have a look at:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

We can use namespace aliases and access the constants through some alias.

[Mads Ager (chromium), 2011-01-20 14:02:14]

I think we should take this opportunity to get rid of all the assembler::
namespaces in the system and just use v8 and v8::internal.

This will be a very nice cleanup. :)

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

http://codereview.chromium.org/6274009/diff/1/src/arm/constants-arm.h#newcode94
src/arm/constants-arm.h:94: namespace assembler {
No that we are sharing all of this why don't we use to opportunity to get rid of
the assembler::arm namespace all together. I don't really see a reason for it.
The simulator should just be in the v8::internal namespace as well.

[Alexandre, 2011-01-21 16:22:14]

Thanks for the review!

Alexandre

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

http://codereview.chromium.org/6274009/diff/1/src/arm/assembler-arm.h#newcode43
src/arm/assembler-arm.h:43: #include "constants-arm.h"
On 2011/01/20 12:40:00, Mads Ager wrote:
> Can we keep the include list alphabeticed?

Done.

http://codereview.chromium.org/6274009/diff/1/src/arm/assembler-arm.h#newcode47
src/arm/assembler-arm.h:47: using namespace assembler::arm;
Removed. We don't need it anymore as the assembler::arm namespace was merged in
v8::internal.
On 2011/01/20 12:40:00, Mads Ager wrote:
> We are not using 'using' directives. Have a look at:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> 
> We can use namespace aliases and access the constants through some alias.

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

http://codereview.chromium.org/6274009/diff/1/src/arm/constants-arm.h#newcode94
src/arm/constants-arm.h:94: namespace assembler {
Done!
This is much more convenient.
On 2011/01/20 14:02:14, Mads Ager wrote:
> No that we are sharing all of this why don't we use to opportunity to get rid
of
> the assembler::arm namespace all together. I don't really see a reason for it.
> The simulator should just be in the v8::internal namespace as well.

[Mads Ager (chromium), 2011-01-24 11:45:03]

This looks great!

A couple of indentation changes and suggestions below. Once they are addressed I
think we are ready to land this.

We are very inconsistent in our enumerator names. I have put in a comment on the
things that I think we should fix before landing this patch. The rest we can
look at in follow up patches.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:432: return ((instr & ~RdMask) == kPushRegPattern);
RdMask is a constant. We should keep the kRdMask name for it.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:491: if ((Instruction::ConditionField(instr) ==
special_condition) &&
kSpecialCondition?

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1462: } else if (Instruction::RdValue(mem_write_instr)
Not your change, but could you format this like the other cases please?

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1470: Instruction::RdValue(ldr_instr)) ||
Align with Instruction::RdValue on the line above.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1599: svc(stop_code + code, cond);
Is stop_code a constant? In that case it should be named as one: kStopCode.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:119: no_condition = -1,
kNoCondition

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:121: eq =  0 << 28,                 // Z set           
Equal.
These really should be capitalized, but in order to not have to change
everything I'm OK with leaving these.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:137: special_condition = 15 << 28,  // Special condition
(refer to section A3.2.1).
kSpecialCondition?

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:138: number_of_conditions = 16,
kNumberOfConditions?

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:256: // Instruction bit masks.
Prefix all of these with a 'k': kCondMask, kALUMask, ...

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:296: number_of_shifts = 4
kNumberOfShifts

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:364: call_rt_redirected = 0x10,
Please use kCamelCase naming for these.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:235: ", %s ", shift_names[shift_index]);
Identation.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:248: "#%d", imm);
Indentation.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:257: ", %s #%d",
Indentation.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:304: "%d - 0x%x",
Indentation.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:309: "%d",
Indentation.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:405: ", #%d", imm);
Indentation. And more occurrences below.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:1358: namespace v8i = v8::internal;
I don't like the v8i shorthand. Remove and just use v8::internal as before.

Alternatively, is there any reason to actually have the disassembler support in
a different namespace. Why not have that in the v8::internal namespace as well?

http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h
File src/arm/simulator-arm.h (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h#new...
src/arm/simulator-arm.h:355: } }  // namespace v8::internal
Remove lines 355-359? Leaving and re-entering the same namespace.

[Alexandre, 2011-01-25 17:19:55]

Just saw a last comment on a few words' case.
I will fix it.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:432: return ((instr & ~RdMask) == kPushRegPattern);
On 2011/01/24 11:45:03, Mads Ager wrote:
> RdMask is a constant. We should keep the kRdMask name for it.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:491: if ((Instruction::ConditionField(instr) ==
special_condition) &&
On 2011/01/24 11:45:03, Mads Ager wrote:
> kSpecialCondition?

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1462: } else if (Instruction::RdValue(mem_write_instr)
On 2011/01/24 11:45:03, Mads Ager wrote:
> Not your change, but could you format this like the other cases please?

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1470: Instruction::RdValue(ldr_instr)) ||
On 2011/01/24 11:45:03, Mads Ager wrote:
> Align with Instruction::RdValue on the line above.

Wasn't exactly sure of the best indentation .
I hope this works.

http://codereview.chromium.org/6274009/diff/21001/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1599: svc(stop_code + code, cond);
It is. Renamed.
On 2011/01/24 11:45:03, Mads Ager wrote:
> Is stop_code a constant? In that case it should be named as one: kStopCode.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:119: no_condition = -1,
On 2011/01/24 11:45:03, Mads Ager wrote:
> kNoCondition
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Enumer...

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:121: eq =  0 << 28,                 // Z set           
Equal.
I leave it then. I think it'll also fit better with the rest of the code and
other archs.
On 2011/01/24 11:45:03, Mads Ager wrote:
> These really should be capitalized, but in order to not have to change
> everything I'm OK with leaving these.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:137: special_condition = 15 << 28,  // Special condition
(refer to section A3.2.1).
Initially named all these to be coherent with lower-case conditions.
I'll use kCamelCase for all non standard conditions.
If we use kNoCondition I think
On 2011/01/24 11:45:03, Mads Ager wrote:
> kSpecialCondition?

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:138: number_of_conditions = 16,
Same. Done.
On 2011/01/24 11:45:03, Mads Ager wrote:
> kNumberOfConditions?

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:256: // Instruction bit masks.
On 2011/01/24 11:45:03, Mads Ager wrote:
> Prefix all of these with a 'k': kCondMask, kALUMask, ...

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:296: number_of_shifts = 4
On 2011/01/24 11:45:03, Mads Ager wrote:
> kNumberOfShifts

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc
File src/arm/disasm-arm.cc (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:235: ", %s ", shift_names[shift_index]);
On 2011/01/24 11:45:03, Mads Ager wrote:
> Identation.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:248: "#%d", imm);
On 2011/01/24 11:45:03, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:257: ", %s #%d",
On 2011/01/24 11:45:03, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:304: "%d - 0x%x",
On 2011/01/24 11:45:03, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:309: "%d",
On 2011/01/24 11:45:03, Mads Ager wrote:
> Indentation.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:405: ", #%d", imm);
On 2011/01/24 11:45:03, Mads Ager wrote:
> Indentation. And more occurrences below.

Done.

http://codereview.chromium.org/6274009/diff/21001/src/arm/disasm-arm.cc#newco...
src/arm/disasm-arm.cc:1358: namespace v8i = v8::internal;
Replaced v8i with v8::internal.

About the disasm namespace, I can't just remove it on ARM. The disasm namespace
is used for all architectures, and there are 2 Disassembler classes (disasm.h
and disassembler.h) distinguished through this namespace.

On 2011/01/24 11:45:03, Mads Ager wrote:
> I don't like the v8i shorthand. Remove and just use v8::internal as before.
> 
> Alternatively, is there any reason to actually have the disassembler support
in
> a different namespace. Why not have that in the v8::internal namespace as
well?

http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h
File src/arm/simulator-arm.h (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/simulator-arm.h#new...
src/arm/simulator-arm.h:355: } }  // namespace v8::internal
On 2011/01/24 11:45:03, Mads Ager wrote:
> Remove lines 355-359? Leaving and re-entering the same namespace.

Done.

[Alexandre, 2011-01-25 17:30:25]

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/6274009/diff/21001/src/arm/constants-arm.h#new...
src/arm/constants-arm.h:364: call_rt_redirected = 0x10,
Fixed.
I missed this one as I nearly missed a few others: some comments only appear in
the side-by-side or only n the unified-diff mode in the reviewing tools...
On 2011/01/24 11:45:03, Mads Ager wrote:
> Please use kCamelCase naming for these.

[Mads Ager (chromium), 2011-01-26 08:23:48]

LGTM. I'll take care of the last identation nit when landing. Thanks!

http://codereview.chromium.org/6274009/diff/40002/src/arm/assembler-arm.cc
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/6274009/diff/40002/src/arm/assembler-arm.cc#ne...
src/arm/assembler-arm.cc:1470: Instruction::RdValue(ldr_instr)) ||
Indentation is off here.