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 [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
