From ac712f13c31581c748fcb70fd5bf46d91bd7f03d Mon Sep 17 00:00:00 2001 From: "mstarzinger@chromium.org" Date: Mon, 17 Oct 2011 07:43:40 +0000 Subject: [PATCH] Fix evaluation order of GT and LTE operators. According to the ES5 spec all ">" and "<=" expressions should be be evaluated left-to-right. This obsoletes old hacks for reversing the order to be ES3 compliant. R=lrn@chromium.org BUG=v8:1752 Review URL: http://codereview.chromium.org/8275035 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@9641 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/full-codegen-arm.cc | 14 +++----------- src/arm/ic-arm.cc | 6 ++---- src/arm/lithium-arm.cc | 6 ++---- src/arm/lithium-codegen-arm.cc | 3 --- src/ia32/full-codegen-ia32.cc | 14 +++----------- src/ia32/ic-ia32.cc | 6 ++---- src/ia32/lithium-codegen-ia32.cc | 3 --- src/ia32/lithium-ia32.cc | 6 ++---- src/x64/full-codegen-x64.cc | 14 +++----------- src/x64/ic-x64.cc | 6 ++---- src/x64/lithium-codegen-x64.cc | 3 --- src/x64/lithium-x64.cc | 6 ++---- test/mjsunit/compiler/compare.js | 4 ++-- test/mjsunit/to_number_order.js | 4 ++-- test/mozilla/mozilla.status | 6 ------ test/sputnik/sputnik.status | 4 ++++ test/test262/test262.status | 13 ------------- 17 files changed, 29 insertions(+), 89 deletions(-) diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index 353ce5b106..1db296a761 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -4071,33 +4071,25 @@ void FullCodeGenerator::VisitCompareOperation(CompareOperation* expr) { case Token::EQ_STRICT: case Token::EQ: cond = eq; - __ pop(r1); break; case Token::LT: cond = lt; - __ pop(r1); break; case Token::GT: - // Reverse left and right sides to obtain ECMA-262 conversion order. - cond = lt; - __ mov(r1, result_register()); - __ pop(r0); + cond = gt; break; case Token::LTE: - // Reverse left and right sides to obtain ECMA-262 conversion order. - cond = ge; - __ mov(r1, result_register()); - __ pop(r0); + cond = le; break; case Token::GTE: cond = ge; - __ pop(r1); break; case Token::IN: case Token::INSTANCEOF: default: UNREACHABLE(); } + __ pop(r1); bool inline_smi_code = ShouldInlineSmiCase(op); JumpPatchSite patch_site(masm_); diff --git a/src/arm/ic-arm.cc b/src/arm/ic-arm.cc index 6e0badca1d..353450d8c9 100644 --- a/src/arm/ic-arm.cc +++ b/src/arm/ic-arm.cc @@ -1559,11 +1559,9 @@ Condition CompareIC::ComputeCondition(Token::Value op) { case Token::LT: return lt; case Token::GT: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return lt; + return gt; case Token::LTE: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return ge; + return le; case Token::GTE: return ge; default: diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index 84959397b6..7da5296043 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -1404,12 +1404,10 @@ LInstruction* LChunkBuilder::DoPower(HPower* instr) { LInstruction* LChunkBuilder::DoCompareGeneric(HCompareGeneric* instr) { - Token::Value op = instr->token(); ASSERT(instr->left()->representation().IsTagged()); ASSERT(instr->right()->representation().IsTagged()); - bool reversed = (op == Token::GT || op == Token::LTE); - LOperand* left = UseFixed(instr->left(), reversed ? r0 : r1); - LOperand* right = UseFixed(instr->right(), reversed ? r1 : r0); + LOperand* left = UseFixed(instr->left(), r1); + LOperand* right = UseFixed(instr->right(), r0); LCmpT* result = new LCmpT(left, right); return MarkAsCall(DefineFixed(result, r0), instr); } diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc index 460573559c..42d1ec955d 100644 --- a/src/arm/lithium-codegen-arm.cc +++ b/src/arm/lithium-codegen-arm.cc @@ -2176,9 +2176,6 @@ void LCodeGen::DoCmpT(LCmpT* instr) { __ cmp(r0, Operand(0)); // This instruction also signals no smi code inlined. Condition condition = ComputeCompareCondition(op); - if (op == Token::GT || op == Token::LTE) { - condition = ReverseCondition(condition); - } __ LoadRoot(ToRegister(instr->result()), Heap::kTrueValueRootIndex, condition); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index 33d5cabad7..3b0c79b7e7 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -4147,33 +4147,25 @@ void FullCodeGenerator::VisitCompareOperation(CompareOperation* expr) { case Token::EQ_STRICT: case Token::EQ: cc = equal; - __ pop(edx); break; case Token::LT: cc = less; - __ pop(edx); break; case Token::GT: - // Reverse left and right sizes to obtain ECMA-262 conversion order. - cc = less; - __ mov(edx, result_register()); - __ pop(eax); + cc = greater; break; case Token::LTE: - // Reverse left and right sizes to obtain ECMA-262 conversion order. - cc = greater_equal; - __ mov(edx, result_register()); - __ pop(eax); + cc = less_equal; break; case Token::GTE: cc = greater_equal; - __ pop(edx); break; case Token::IN: case Token::INSTANCEOF: default: UNREACHABLE(); } + __ pop(edx); decrement_stack_height(); bool inline_smi_code = ShouldInlineSmiCase(op); diff --git a/src/ia32/ic-ia32.cc b/src/ia32/ic-ia32.cc index 8a98b179d3..57c9982537 100644 --- a/src/ia32/ic-ia32.cc +++ b/src/ia32/ic-ia32.cc @@ -1591,11 +1591,9 @@ Condition CompareIC::ComputeCondition(Token::Value op) { case Token::LT: return less; case Token::GT: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return less; + return greater; case Token::LTE: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return greater_equal; + return less_equal; case Token::GTE: return greater_equal; default: diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 35fa618f80..51e0828047 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -2029,9 +2029,6 @@ void LCodeGen::DoCmpT(LCmpT* instr) { CallCode(ic, RelocInfo::CODE_TARGET, instr); Condition condition = ComputeCompareCondition(op); - if (op == Token::GT || op == Token::LTE) { - condition = ReverseCondition(condition); - } Label true_value, done; __ test(eax, Operand(eax)); __ j(condition, &true_value, Label::kNear); diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index 856106c799..997f17e2f5 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -1434,13 +1434,11 @@ LInstruction* LChunkBuilder::DoPower(HPower* instr) { LInstruction* LChunkBuilder::DoCompareGeneric(HCompareGeneric* instr) { - Token::Value op = instr->token(); ASSERT(instr->left()->representation().IsTagged()); ASSERT(instr->right()->representation().IsTagged()); - bool reversed = (op == Token::GT || op == Token::LTE); LOperand* context = UseFixed(instr->context(), esi); - LOperand* left = UseFixed(instr->left(), reversed ? eax : edx); - LOperand* right = UseFixed(instr->right(), reversed ? edx : eax); + LOperand* left = UseFixed(instr->left(), edx); + LOperand* right = UseFixed(instr->right(), eax); LCmpT* result = new LCmpT(context, left, right); return MarkAsCall(DefineFixed(result, eax), instr); } diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index b5c5fc5e71..39dfe842a3 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -3997,33 +3997,25 @@ void FullCodeGenerator::VisitCompareOperation(CompareOperation* expr) { case Token::EQ_STRICT: case Token::EQ: cc = equal; - __ pop(rdx); break; case Token::LT: cc = less; - __ pop(rdx); break; case Token::GT: - // Reverse left and right sizes to obtain ECMA-262 conversion order. - cc = less; - __ movq(rdx, result_register()); - __ pop(rax); + cc = greater; break; case Token::LTE: - // Reverse left and right sizes to obtain ECMA-262 conversion order. - cc = greater_equal; - __ movq(rdx, result_register()); - __ pop(rax); + cc = less_equal; break; case Token::GTE: cc = greater_equal; - __ pop(rdx); break; case Token::IN: case Token::INSTANCEOF: default: UNREACHABLE(); } + __ pop(rdx); bool inline_smi_code = ShouldInlineSmiCase(op); JumpPatchSite patch_site(masm_); diff --git a/src/x64/ic-x64.cc b/src/x64/ic-x64.cc index 27a96674cf..c79d44dd7c 100644 --- a/src/x64/ic-x64.cc +++ b/src/x64/ic-x64.cc @@ -1613,11 +1613,9 @@ Condition CompareIC::ComputeCondition(Token::Value op) { case Token::LT: return less; case Token::GT: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return less; + return greater; case Token::LTE: - // Reverse left and right operands to obtain ECMA-262 conversion order. - return greater_equal; + return less_equal; case Token::GTE: return greater_equal; default: diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc index 6b3a73a714..8d806e6ad7 100644 --- a/src/x64/lithium-codegen-x64.cc +++ b/src/x64/lithium-codegen-x64.cc @@ -1979,9 +1979,6 @@ void LCodeGen::DoCmpT(LCmpT* instr) { CallCode(ic, RelocInfo::CODE_TARGET, instr); Condition condition = TokenToCondition(op, false); - if (op == Token::GT || op == Token::LTE) { - condition = ReverseCondition(condition); - } Label true_value, done; __ testq(rax, rax); __ j(condition, &true_value, Label::kNear); diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index a67a59320e..8c21c2d50e 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -1396,12 +1396,10 @@ LInstruction* LChunkBuilder::DoPower(HPower* instr) { LInstruction* LChunkBuilder::DoCompareGeneric(HCompareGeneric* instr) { - Token::Value op = instr->token(); ASSERT(instr->left()->representation().IsTagged()); ASSERT(instr->right()->representation().IsTagged()); - bool reversed = (op == Token::GT || op == Token::LTE); - LOperand* left = UseFixed(instr->left(), reversed ? rax : rdx); - LOperand* right = UseFixed(instr->right(), reversed ? rdx : rax); + LOperand* left = UseFixed(instr->left(), rdx); + LOperand* right = UseFixed(instr->right(), rax); LCmpT* result = new LCmpT(left, right); return MarkAsCall(DefineFixed(result, rax), instr); } diff --git a/test/mjsunit/compiler/compare.js b/test/mjsunit/compiler/compare.js index 3f96087002..460b0ab003 100644 --- a/test/mjsunit/compiler/compare.js +++ b/test/mjsunit/compiler/compare.js @@ -83,9 +83,9 @@ function TestNonPrimitive(order, f) { } TestNonPrimitive("xy", MaxLT); -TestNonPrimitive("yx", MaxLE); +TestNonPrimitive("xy", MaxLE); TestNonPrimitive("xy", MaxGE); -TestNonPrimitive("yx", MaxGT); +TestNonPrimitive("xy", MaxGT); // Test compare in case of aliased registers. function CmpX(x) { if (x == x) return 42; } diff --git a/test/mjsunit/to_number_order.js b/test/mjsunit/to_number_order.js index d17e600050..50e4bc762e 100644 --- a/test/mjsunit/to_number_order.js +++ b/test/mjsunit/to_number_order.js @@ -161,7 +161,7 @@ assertEquals("fiskfisk", x, "Compare objects b >= b valueOf order"); x = ""; assertFalse(a > b, "Compare objects a > b"); -assertEquals("fiskhest", x, "Compare objects a > b valueOf order"); +assertEquals("hestfisk", x, "Compare objects a > b valueOf order"); x = ""; assertFalse(a > void(0), "Compare objects a > undefined"); @@ -195,7 +195,7 @@ function identical_object_comparison() { x = ""; assertFalse(a > b, "Compare objects a > b"); - assertEquals("fiskhest", x, "Compare objects a > b valueOf order"); + assertEquals("hestfisk", x, "Compare objects a > b valueOf order"); x = ""; assertFalse(a > void(0), "Compare objects a > undefined"); diff --git a/test/mozilla/mozilla.status b/test/mozilla/mozilla.status index 6a5c08640c..6d3d0cab9e 100644 --- a/test/mozilla/mozilla.status +++ b/test/mozilla/mozilla.status @@ -410,12 +410,6 @@ js1_5/extensions/regress-435345-01: FAIL_OK js1_5/extensions/regress-455413: FAIL_OK -# The spec specifies reverse evaluation order for < and >=. -# See section 11.8.2 and 11.8.5. -# We implement the spec here but the test tests the more straigtforward order. -ecma_3/Operators/order-01: FAIL_OK - - # Uses Mozilla-specific QName, XML, XMLList and Iterator. js1_5/Regress/regress-407323: FAIL_OK js1_5/Regress/regress-407957: FAIL_OK diff --git a/test/sputnik/sputnik.status b/test/sputnik/sputnik.status index 99db598af4..ac02d25e6a 100644 --- a/test/sputnik/sputnik.status +++ b/test/sputnik/sputnik.status @@ -162,6 +162,10 @@ S11.1.5_A4.2: FAIL_OK S9.9_A1: FAIL_OK S9.9_A2: FAIL_OK +# The expected evaluation order of comparison operations changed. +S11.8.2_A2.3_T1: FAIL_OK +S11.8.3_A2.3_T1: FAIL_OK + # Calls builtins without an explicit receiver which means that # undefined is passed to the builtin. The tests expect the global # object to be passed which was true in ES3 but not in ES5. diff --git a/test/test262/test262.status b/test/test262/test262.status index 1a619547d7..f871dd0e19 100644 --- a/test/test262/test262.status +++ b/test/test262/test262.status @@ -43,19 +43,6 @@ S8.7_A5_T2: FAIL # V8 Bug: http://code.google.com/p/v8/issues/detail?id=1624 S10.4.2.1_A1: FAIL -# V8 Bug: http://code.google.com/p/v8/issues/detail?id=1752 -S11.8.2_A2.3_T1: FAIL -S11.8.3_A2.3_T1: FAIL -11.8.2-1: FAIL -11.8.2-2: FAIL -11.8.2-3: FAIL -11.8.2-4: FAIL -11.8.3-1: FAIL -11.8.3-2: FAIL -11.8.3-3: FAIL -11.8.3-4: FAIL -11.8.3-5: FAIL - # V8 Bug. S13.2.3_A1: FAIL