2010/4/6 <[email protected]> > Hi Erik, > > Thank you for the reviews. We merged the changes with the newer bleeding > edge > (svn 4311) to > 1. take care of the function calls generated in macro-assembler-arm.cc; > 2. use the existing ARMv7 feature; > 3. re-evalute the benefit. > > Most of the comments and concerns are addressed. The regression test > (except the > serialization tests) passes. We see about 2.6% improvement on SunSpider and > v8-suite. In the current changes, I think we don't need CPU::FlushICache in > set_target_address_at. > 1. set_target_address_at is called by set_target_object and > set_target_address. > 2. set_target_object is called in objects.cc followed with > "CPU::FlushICache(instruction_start(), instruction_size())". > 3. calls to set_target_address > 3.1 the function call to set_target_address in objects.cc is also followed > by a > FlushICache. >
The region of memory flushed by that call to FlushICache is not the same as the region of memory written by the call to set_target_address. > 3.2 the function call to set_target_address in ic-inh.h is to update the > target > of a function call. In macro-assembler-arm.cc, we remain the use of "ldr + > blx"; > no movw/movt is generated for this case. Thus, set_target_address will > update > the const pool, not the ICache. > It is important that we assert that this is really the case. One way to do that might be to split set_target_address into two versions, one called set_target_address and the other called set_target_address_noflush. In one of them we do the flush and in the other we assert that we are patching the constant pool and not the instruction. Things still have to compile on other platforms, including valgrind-on-ia32 where the cache flush instructions are used to notify valgrind that code has changed. > 3.3 set_target_address is called in debug.cc, mark-compact.cc, and > liveedit.cc. > Currently, we're not quite clear about what those function calls do and how > to > triger those function calls. If you have any insights on this, please let > us > know. > They should be triggered by running the debug tests. They are used for debugging. You have to compile with -DENABLE_DEBUGGER_SUPPORT for this code to be excercised. > > -Kun > > > 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); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode91 > src/arm/assembler-arm-inl.h:91: Instr instr_movt = Memory::int32_at(pc + > 4); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode92 > src/arm/assembler-arm-inl.h:92: if ((instr_movt & 0xff00000) == > 0x3400000) { > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode103 > src/arm/assembler-arm-inl.h:103: if ((instr & 0x0f7f0000) == 0x051f0000) > { > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode268 > src/arm/assembler-arm-inl.h:268: if ((instr & 0x0f7f0000) == 0x051f0000) > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode286 > src/arm/assembler-arm-inl.h:286: // movw movt > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode287 > src/arm/assembler-arm-inl.h:287: uint32_t *new_pc = (unsigned int*)pc; > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/5#newcode291 > src/arm/assembler-arm-inl.h:291: if ((*(new_pc+1) & 0xff00000) == > 0x3400000) { > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > 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: > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/6#newcode635 > src/arm/assembler-arm.cc:635: Condition cond = > static_cast<Condition>(instr & CondMask); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/6#newcode636 > src/arm/assembler-arm.cc:636: if ((x.rmode_ != > RelocInfo::EMBEDDED_OBJECT) && > On 2010/03/22 11:50:09, Erik Corry wrote: > > EMBEDDED_OBJECT causes failures. The reason is unknown, so it's disabled > now. > > http://codereview.chromium.org/1128009/diff/1/6#newcode642 > src/arm/assembler-arm.cc:642: if (x.imm32_ > 65535) > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > 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 > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/6#newcode967 > src/arm/assembler-arm.cc:967: | dst.code()*B12 | (((src.imm32_ >> 16)& > 0xfff))); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/6#newcode1714 > src/arm/assembler-arm.cc:1714: bool withconstpool) { > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > 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)) != > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/9 > File src/arm/disasm-arm.cc (right): > > http://codereview.chromium.org/1128009/diff/1/9#newcode664 > src/arm/disasm-arm.cc:664: > //------------------------------------------------------------ > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/9#newcode703 > src/arm/disasm-arm.cc:703: Format(instr, "movw'cond 'mw"); // movw > instruction > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/9#newcode729 > src/arm/disasm-arm.cc:729: Format(instr, "movt'cond 'mt"); // movt > instruction > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > 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"); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/8#newcode1532 > src/arm/simulator-arm.cc:1532: alu_out = (instr->Immed4Field() << 12) | > instr->Offset12Field(); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/8#newcode1574 > src/arm/simulator-arm.cc:1574: // Format(instr, "movt'cond 'rd, 'rn, > 'imm"); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/8#newcode2039 > src/arm/simulator-arm.cc:2039: int offset_8 = 0x000000FF & > instr->Bits(7, 0); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/8#newcode2094 > src/arm/simulator-arm.cc:2094: /*if((unsigned int) instr == 0x579241ac) > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > 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()); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/3#newcode5021 > src/objects.cc:5021: if ((instr_movt & 0xff00000) == 0x3400000) { > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/3#newcode5022 > src/objects.cc:5022: unsigned int offset_movt = ((instr_movt & 0xf0000) > >> 4) | >>> >> On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009/diff/1/3#newcode5027 > src/objects.cc:5027: Code** p = reinterpret_cast<Code **>(p1); > On 2010/03/22 11:50:09, Erik Corry wrote: > > Done. > > http://codereview.chromium.org/1128009 > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe, reply using "remove me" as the subject.
