Fix invalid X87 stack depth after LCompareNumericAndBranch.

Fix invalid X87 stack depth after LCompareNumericAndBranch.

R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=16909

[Benedikt Meurer, 2013-09-23 13:48:53]

Hey Toon,
This fixes a bug where the X87 stack was in inconsistent state after
LCompareNumericAndBranch depending on whether the then or else branch was
executed. The "then" branch had the 2 arguments popped off, while the "else"
branch had the 2 arguments left on stack. This way the LCompareNumericAndBranch
instruction itself cleans up the stack, i.e. pops both arguments from the X87
stack, which is what the other control instructions do (i.e. LCmpHoleAndBranch).
PTAL
-- Benedikt

[Toon Verwaest, 2013-09-24 09:00:08]

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:570: x87_stack_.pop();
Pretty surprising that you first push values to them immediately pop. Can't we
let FCmp consume these values to avoid direct dependency between these
instructions?

[Toon Verwaest, 2013-09-24 09:02:02]

lgtm

[oliv, 2013-09-24 09:03:14]

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:418: !instr->IsGap() && !instr->IsReturn()) {
It would be nice to document the invariant this cl restores, which is afaik:

if (instr->ClobbersDoubleRegisters()) {
  ASSERT(x87_stack_.depth() ==
         instr->HasDoubleRegisterResult() ? 1 : 0);
}

[Benedikt Meurer, 2013-09-24 09:12:30]

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:570: x87_stack_.pop();
On 2013/09/24 09:00:08, Toon Verwaest wrote:
> Pretty surprising that you first push values to them immediately pop. Can't we
> let FCmp consume these values to avoid direct dependency between these
> instructions?

As discussed offline, we can fix this once the X87 stack tracking is part of the
macro assembler.

[Benedikt Meurer, 2013-09-24 09:26:48]

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/23456044/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:418: !instr->IsGap() && !instr->IsReturn()) {
On 2013/09/24 09:03:15, oliv wrote:
> It would be nice to document the invariant this cl restores, which is afaik:
> 
> if (instr->ClobbersDoubleRegisters()) {
>   ASSERT(x87_stack_.depth() ==
>          instr->HasDoubleRegisterResult() ? 1 : 0);
> }

Done.

[Benedikt Meurer, 2013-09-24 09:29:09]

Committed patchset #2 manually as r16909 (presubmit successful).