As mentioned in email our measurements indicate that inline cache calls are
updated very frequently in the startup phase of a new web page and therefore
they probably have to stay as constant pool loads. A constant pool load
can be
updated without flushing the instruction cache. Unless your own
measurements on
page-cycler-like loads contradict this we have to make sure that IC calls
are
still done with the constant pool. Given this, it's an open question
whether or
not this is still worth it. If it is worth it then here are some comments
on
the code so far.
If it is determined that it is worthwhile then note that Android builds are
typically done without the debugger, so we don't have to fix the debugger
support before landing this, as long as no movw or movt instructions are
generated if there is a debugger. We don't want to have ugly ifdefs any
more
places than we already have, but if the ARMv7 availability check and the
debugger presence check are rolled into one test then that shouldn't be a
problem.
http://codereview.chromium.org/1128009/diff/1/5
File src/arm/assembler-arm-inl.h (right):
http://codereview.chromium.org/1128009/diff/1/5#newcode89
src/arm/assembler-arm-inl.h:89: Instr instr = Memory::int32_at(pc);
You should assert that these two instructions actually are movw and movt
instructions. See kLdrPCMask and lLdrPCPattern for examples on how to
do this.
http://codereview.chromium.org/1128009/diff/1/5#newcode91
src/arm/assembler-arm-inl.h:91: Instr instr_movt = Memory::int32_at(pc +
4);
Instead of 4 you should write kInstrSize. We frown on unnamed constants
in the V8code.
http://codereview.chromium.org/1128009/diff/1/5#newcode92
src/arm/assembler-arm-inl.h:92: if ((instr_movt & 0xff00000) ==
0x3400000) {
This makes no sense. If we don't know for sure that it's a movw/movt
pair then why are we getting the address?
http://codereview.chromium.org/1128009/diff/1/5#newcode103
src/arm/assembler-arm-inl.h:103: if ((instr & 0x0f7f0000) == 0x051f0000)
{
These should have names like kLdrPCMask and kLdrPCPattern.
http://codereview.chromium.org/1128009/diff/1/5#newcode268
src/arm/assembler-arm-inl.h:268: if ((instr & 0x0f7f0000) == 0x051f0000)
Please use {} in multiline if statements.
http://codereview.chromium.org/1128009/diff/1/5#newcode286
src/arm/assembler-arm-inl.h:286: // movw movt
Please use sentences or at least noun phrases with capitalization and
full stops.
http://codereview.chromium.org/1128009/diff/1/5#newcode287
src/arm/assembler-arm-inl.h:287: uint32_t *new_pc = (unsigned int*)pc;
We don't allow C-style casts in Google C++ code.
Also, the asterisk goes next to the type, not next to the variable in
declarations.
http://codereview.chromium.org/1128009/diff/1/5#newcode291
src/arm/assembler-arm-inl.h:291: if ((*(new_pc+1) & 0xff00000) ==
0x3400000) {
Please assert that the previous instruction is movt instead of checking
that it is. If there is no movt there then we can't patch. We can't
use a single movw instruction if we need to be able to patch with an
arbitrary value.
http://codereview.chromium.org/1128009/diff/1/5#newcode298
src/arm/assembler-arm-inl.h:298: // Intuitively, we would think it is
necessary to flush the instruction cache
This comment is important. Your change means it is now necessary to
flush the icache.
http://codereview.chromium.org/1128009/diff/1/6
File src/arm/assembler-arm.cc (right):
http://codereview.chromium.org/1128009/diff/1/6#newcode69
src/arm/assembler-arm.cc:69:
This looks like an accidental edit.
http://codereview.chromium.org/1128009/diff/1/6#newcode635
src/arm/assembler-arm.cc:635: Condition cond =
static_cast<Condition>(instr & CondMask);
You need to check here whether ARMv7 is available and only use it if so.
if (CpuFeatures::IsSupported(ARMv7)))
http://codereview.chromium.org/1128009/diff/1/6#newcode636
src/arm/assembler-arm.cc:636: if ((x.rmode_ !=
RelocInfo::EMBEDDED_OBJECT) &&
It's not clear to me why EMBEDDED_OBJECT is special here.
http://codereview.chromium.org/1128009/diff/1/6#newcode642
src/arm/assembler-arm.cc:642: if (x.imm32_ > 65535)
You need to lose the 'if'. If we have MustUseIp then we can't know
whether the object will move to an address above 65535.
http://codereview.chromium.org/1128009/diff/1/6#newcode966
src/arm/assembler-arm.cc:966: emit(cond | (0x34)*B20 | ((src.imm32_ >>
16 >> 12) & 0xf)*B16
Shifting twice doesn't make things clearer and putting a hex constant in
() doesn't add anything either.
http://codereview.chromium.org/1128009/diff/1/6#newcode967
src/arm/assembler-arm.cc:967: | dst.code()*B12 | (((src.imm32_ >> 16)&
0xfff)));
Space before &.
Using double brackets like this: ((expression)) doesn't make things
easier to read.
http://codereview.chromium.org/1128009/diff/1/6#newcode1714
src/arm/assembler-arm.cc:1714: bool withconstpool) {
Arguments should be named_like_this.
http://codereview.chromium.org/1128009/diff/1/6#newcode1822
src/arm/assembler-arm.cc:1822: if (((instr & (7*B25 | P | U | B | W |
15*B16 | Off12Mask)) !=
This if needs at least a comment to explain what it is doing.
http://codereview.chromium.org/1128009/diff/1/9
File src/arm/disasm-arm.cc (right):
http://codereview.chromium.org/1128009/diff/1/9#newcode382
src/arm/disasm-arm.cc:382: ", #%d", imm);
I think it would be more readable if the movt instruction printed out
#xxxx0000. What do the ARM disassemblers print for this?
http://codereview.chromium.org/1128009/diff/1/9#newcode664
src/arm/disasm-arm.cc:664:
//------------------------------------------------------------
This looks like an accidental edit.
http://codereview.chromium.org/1128009/diff/1/9#newcode703
src/arm/disasm-arm.cc:703: Format(instr, "movw'cond 'mw"); // movw
instruction
The comment doesn't seem to add anything here.
http://codereview.chromium.org/1128009/diff/1/9#newcode729
src/arm/disasm-arm.cc:729: Format(instr, "movt'cond 'mt"); // movt
instruction
Or here.
http://codereview.chromium.org/1128009/diff/1/8
File src/arm/simulator-arm.cc (right):
http://codereview.chromium.org/1128009/diff/1/8#newcode1531
src/arm/simulator-arm.cc:1531: // Format(instr, "movw'cond 'rd, 'rn,
'imm");
Is there really an rn in this instruction?
http://codereview.chromium.org/1128009/diff/1/8#newcode1532
src/arm/simulator-arm.cc:1532: alu_out = (instr->Immed4Field() << 12) |
instr->Offset12Field();
This seems to be done a lot of places. Perhaps instead of adding an
Immed4Field method we should add an ImmedMovtMovwField method that gets
all 16 bits.
http://codereview.chromium.org/1128009/diff/1/8#newcode1574
src/arm/simulator-arm.cc:1574: // Format(instr, "movt'cond 'rd, 'rn,
'imm");
rn?
http://codereview.chromium.org/1128009/diff/1/8#newcode2039
src/arm/simulator-arm.cc:2039: int offset_8 = 0x000000FF &
instr->Bits(7, 0);
Surely the bits 7 to 0 are already in the range 0 - 255, so anding with
0x000000ff won't make any difference?
http://codereview.chromium.org/1128009/diff/1/8#newcode2094
src/arm/simulator-arm.cc:2094: /*if((unsigned int) instr == 0x579241ac)
Accidental edit?
http://codereview.chromium.org/1128009/diff/1/3
File src/objects.cc (right):
http://codereview.chromium.org/1128009/diff/1/3#newcode5012
src/objects.cc:5012: Instr instr = Memory::int32_at(it.rinfo()->pc());
We can't have ARM-specific code in this file since it is used for all
architectures. Some refactoring will be necessary.
http://codereview.chromium.org/1128009/diff/1/3#newcode5021
src/objects.cc:5021: if ((instr_movt & 0xff00000) == 0x3400000) {
Same comment as above on the movt not being optional.
http://codereview.chromium.org/1128009/diff/1/3#newcode5022
src/objects.cc:5022: unsigned int offset_movt = ((instr_movt & 0xf0000)
4) |
Here is another place where my proposed new method could be used.
http://codereview.chromium.org/1128009/diff/1/3#newcode5027
src/objects.cc:5027: Code** p = reinterpret_cast<Code **>(p1);
I wonder why we have to do this in two steps?
http://codereview.chromium.org/1128009
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply
to this email with the words "REMOVE ME" as the subject.
