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