Dean,
The last_pc_ is to track where the last instruction x86 instruction
started. If you can tell me how I can disassemble x86 code backwards I
would be happy to have ARM and x86 code more aligned. Until then I am
fine with this difference as the code is obviously machine dependent.
Cheers,
-Ivan
On Mon, Sep 29, 2008 at 14:46, Dean McNamee <[EMAIL PROTECTED]> wrote:
> Sorry, I think I was incorrect. I was confused between how ARM and
> x86 diverge on handling this. On arm the eliminators rely on
> last_bound_pos_. It seems that on x86, we set last_pc_ = NULL on
> bind(), so that this check will fail:
>
> if (FLAG_push_pop_elimination && (reloc_info_writer.last_pc() <= last_pc_)) {
>
> This totally threw me off, because it seems like this line is just
> checking to make sure we aren't screwing up relocation information,
> but we are also using a NULL last_pc_ to signal that it's not safe to
> do instruction elimination. This is kinda flimsy, I wonder if there
> are any problems with it. I verified that what I said above wasn't
> true though, simple code like this:
>
> __ jmp(&blah);
> __ j(equal, &blahjmp);
> __ push(Immediate(4));
> __ bind(&blahjmp);
> __ pop(eax);
> __ bind(&blah);
>
>
> Will not have the push/pop eliminated. Sorry for the confusion.
> Seems like maybe it would be good to try to make the ARM and x86 cases
> closer here?
>
> On Mon, Sep 29, 2008 at 2:00 PM, Kevin Millikin <[EMAIL PROTECTED]> wrote:
>> That looks bad.
>> I am working on changing to the code generator so it just doesn't generate
>> the push/pop pair (for literals, initially) in the first place. We should
>> probably fix this now, though.
>>
>> On Mon, Sep 29, 2008 at 1:55 PM, Dean McNamee <[EMAIL PROTECTED]> wrote:
>>>
>>> I am not sure this code is correct. On ARM, the push / pop eliminator
>>> looks and make sure that there wasn't a label bound in between the
>>> instructions. For example:
>>>
>>> Label some_label;
>>> j(&some_label);
>>> push 4
>>> bind(&some_label);
>>> pop eax
>>>
>>> The x86 jump eliminator doesn't seem to recognize this case, and will
>>> eliminate the push/pop pair into a single MOV instruction, which is
>>> clearly incorrect. Seems a problem with the rest of the elimination
>>> code also? There is a guard for making sure there is nothing with
>>> relocation information, but there isn't reloc information for a
>>> relative jump, so it would seem these optimizations are unsafe.
>>>
>>> On Tue, Sep 23, 2008 at 3:28 PM, Kasper Lund <[EMAIL PROTECTED]> wrote:
>>> >
>>> > LGTM even though I hope the way the peephole stuff is written could be
>>> > improved. It's growing a lot - it would be nice with some
>>> > abstractions...
>>> >
>>> > On Tue, Sep 23, 2008 at 3:23 PM, <[EMAIL PROTECTED]> wrote:
>>> >> Reviewers: Kasper Lund,
>>> >>
>>> >> Description:
>>> >> Added some peephole optimizaitions regarding push of immediate followed
>>> >> by pop eax.
>>> >>
>>> >> Please review this at http://codereview.chromium.org/4212
>>> >>
>>> >> Affected files:
>>> >> M src/assembler-ia32.cc
>>> >>
>>> >>
>>> >> Index: src/assembler-ia32.cc
>>> >> ===================================================================
>>> >> --- src/assembler-ia32.cc (revision 360)
>>> >> +++ src/assembler-ia32.cc (working copy)
>>> >> @@ -469,7 +469,48 @@
>>> >> }
>>> >> return;
>>> >> }
>>> >> + } else if (instr == 0x6a && dst.is(eax)) { // push of immediate 8
>>> >> bit
>>> >> + byte imm8 = last_pc_[1];
>>> >> + if (imm8 == 0) {
>>> >> + // 6a00 push 0x0
>>> >> + // 58 pop eax
>>> >> + last_pc_[0] = 0x31;
>>> >> + last_pc_[1] = 0xc0;
>>> >> + // change to
>>> >> + // 31c0 xor eax,eax
>>> >> + last_pc_ = NULL;
>>> >> + return;
>>> >> + } else {
>>> >> + // 6a00 push 0xXX
>>> >> + // 58 pop eax
>>> >> + last_pc_[0] = 0xb8;
>>> >> + EnsureSpace ensure_space(this);
>>> >> + if ((imm8 & 0x80) != 0) {
>>> >> + EMIT(0xff);
>>> >> + EMIT(0xff);
>>> >> + EMIT(0xff);
>>> >> + // change to
>>> >> + // b8XXffffff mov eax,0xffffffXX
>>> >> + } else {
>>> >> + EMIT(0x00);
>>> >> + EMIT(0x00);
>>> >> + EMIT(0x00);
>>> >> + // change to
>>> >> + // b8XX000000 mov eax,0x000000XX
>>> >> + }
>>> >> + last_pc_ = NULL;
>>> >> + return;
>>> >> + }
>>> >> + } else if (instr == 0x68 && dst.is(eax)) { // push of immediate
>>> >> 32 bit
>>> >> + // 68XXXXXXXX push 0xXXXXXXXX
>>> >> + // 58 pop eax
>>> >> + last_pc_[0] = 0xb8;
>>> >> + last_pc_ = NULL;
>>> >> + // change to
>>> >> + // b8XXXXXXXX mov eax,0xXXXXXXXX
>>> >> + return;
>>> >> }
>>> >> +
>>> >> // Other potential patterns for peephole:
>>> >> // 0x712716 102 890424 mov [esp], eax
>>> >> // 0x712719 105 8b1424 mov edx, [esp]
>>> >>
>>> >>
>>> >>
>>> >
>>> > >
>>> >
>>>
>>>
>>
>>
>>
>> --
>> Google Denmark ApS
>> CVR nr. 28 86 69 84
>> c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K,
>> Denmark
>>
>> >>
>>
>
--~--~---------~--~----~------------~-------~--~----~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~----------~----~----~----~------~----~------~--~---
