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 -~----------~----~----~----~------~----~------~--~---