The change needs to be brought up to date so that it is relative to the
bleeding
edge. Due to limitations in the codereview tool this probably means
creating a
new issue on codereview.chromium.org.
http://codereview.chromium.org/1128009/diff/46001/47003
File src/arm/assembler-arm-inl.h (right):
http://codereview.chromium.org/1128009/diff/46001/47003#newcode97
src/arm/assembler-arm-inl.h:97: uint32_t offset = ((instr & 0xf0000) >>
4) | (instr & 0xfff);
The name 'offset' seems wrong here. It's the immediate from the
movw-movt instruction. The word 'offset' makes it sound like it is
relative to something.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode110
src/arm/assembler-arm-inl.h:110: return
reinterpret_cast<Object**>(get_movw_movt_address(pc_));
This seems wrong. This function is expected to return a Handle, which
is implemented internally as a pointer to a pointer to an object. This
line takes a pointer to and object and casts it to add the extra
indirection. Incidentally this line makes use of the implicit Handle
constructor that takes an Object**. I have made that constructor
explicit since it violated our style guide.
I think the correct change is to have this function return an Object*
instead of a Handle<Object>. It seems like the only place we use this
(objects.cc) we immediately get the Object* out of the Handle anyway.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode123
src/arm/assembler-arm-inl.h:123: Assembler::set_target_address_at(pc_,
reinterpret_cast<Address>(target),
If all arguments don't fit on one line it is neater to give them one
line each so that it visually looks like 3 arguments and not two.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode298
src/arm/assembler-arm-inl.h:298: void
Assembler::set_target_address_at(Address pc, Address target,
Please put arguments either on one line or on a line each.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode307
src/arm/assembler-arm-inl.h:307: *new_pc = (*new_pc & 0xfff0f000) |
((new_addr & 0xf000) << 4) |
Please give this constant, 0xfff0ff000, a name and use it throughout.
Also, there is a function that returns ((x & 0xf000) << 4) | (x &
0xfff). Please use it throughout.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode319
src/arm/assembler-arm-inl.h:319: if (need_icache_flush)
Please use braces on multiline if statements. This isn't a Google style
guide requirement, but we try to stick to it on V8.
http://codereview.chromium.org/1128009/diff/46001/47003#newcode320
src/arm/assembler-arm-inl.h:320: CPU::FlushICache(pc, sizeof(target));
sizeof(target) looks wrong here. With the loop above, the size to be
flushed is not a constant.
http://codereview.chromium.org/1128009/diff/46001/47004
File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/1128009/diff/46001/47004#newcode590
src/arm/assembler-arm.cc:590: movt(rd, x, cond);
In the case of the relocinfo being "None" we don't need to emit the movt
unless it is non-zero. The RelocInfo class in assembler.h seems to be
missing an IsNone() method, but it is easy to add.
http://codereview.chromium.org/1128009/diff/46001/47004#newcode602
src/arm/assembler-arm.cc:602: ldr(ip, MemOperand(pc, 0), cond);
This code, that handles instructions other than mov, is missing in the
movw_movt branch of this instruction. If you add it back then perhaps
you don't need the use_movw_movt argument any more?
http://codereview.chromium.org/1128009/diff/46001/47004#newcode1954
src/arm/assembler-arm.cc:1954: int block_for_prinfo =
num_prinfo_*kInstrSize;
There should be spaces around '*'.
http://codereview.chromium.org/1128009/diff/46001/47004#newcode1961
src/arm/assembler-arm.cc:1961: if (((instr & (7*B25 | P | U | B | W |
15*B16 | Off12Mask)) !=
It must be possible to rewrite this with a named mask and a named value.
http://codereview.chromium.org/1128009/diff/46001/47004#newcode1991
src/arm/assembler-arm.cc:1991: if (((instr & (7*B25 | P | U | B | W |
15*B16 | Off12Mask)) !=
And this.
http://codereview.chromium.org/1128009/diff/46001/47004#newcode1993
src/arm/assembler-arm.cc:1993: continue;
Use braces on multi-line ifs.
http://codereview.chromium.org/1128009/diff/46001/47002
File src/arm/assembler-arm.h (right):
http://codereview.chromium.org/1128009/diff/46001/47002#newcode715
src/arm/assembler-arm.h:715: SBit s = LeaveCC, Condition cond = al, bool
use_movw_movt = true);
Having bool arguments makes the code unreadable at the call site. We
prefer enums like SBit.
http://codereview.chromium.org/1128009/diff/46001/47002#newcode1085
src/arm/assembler-arm.h:1085: bool with_const_pool = true);
Also here the bool argument makes code unreadble and should be replaced
with an enum.
http://codereview.chromium.org/1128009/diff/46001/47007
File src/arm/disasm-arm.cc (right):
http://codereview.chromium.org/1128009/diff/46001/47007#newcode386
src/arm/disasm-arm.cc:386: ", #%d0000", imm);
This line has wrong alignment.
http://codereview.chromium.org/1128009/diff/46001/47008
File src/arm/macro-assembler-arm.cc (right):
http://codereview.chromium.org/1128009/diff/46001/47008#newcode130
src/arm/macro-assembler-arm.cc:130: mov(ip, Operand(target, rmode),
LeaveCC, cond, false);
There should be a comment here to explain why we don't want to use movw
and movt.
http://codereview.chromium.org/1128009/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
