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