Changeset 247011 in webkit

Timestamp:

Jul 1, 2019 9:59:33 AM (5 years ago)

Author:

Ryan Haddad

Message:

Unreviewed, rolling out r246946.

Caused JSC test crashes on arm64

Reverted changeset:

"Add b3 macro lowering for CheckMul on arm64"
​https://bugs.webkit.org/show_bug.cgi?id=199251
​https://trac.webkit.org/changeset/246946

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/check-mul-constant.js (deleted)
• JSTests/microbenchmarks/check-mul-no-constant.js (deleted)
• JSTests/microbenchmarks/check-mul-power-of-two.js (deleted)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/assembler/MacroAssemblerARM64.h (modified) (1 diff)
• Source/JavaScriptCore/assembler/testmasm.cpp (modified) (2 diffs)
• Source/JavaScriptCore/b3/B3LowerMacros.cpp (modified) (1 diff)
• Source/JavaScriptCore/b3/B3LowerToAir.cpp (modified) (1 diff)
• Source/JavaScriptCore/b3/air/AirOpcode.opcodes (modified) (1 diff)
• Source/JavaScriptCore/b3/testb3.cpp (modified) (10 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-07-01  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r246946.
+
+        Caused JSC test crashes on arm64
+
+        Reverted changeset:
+
+        "Add b3 macro lowering for CheckMul on arm64"
+        https://bugs.webkit.org/show_bug.cgi?id=199251
+        https://trac.webkit.org/changeset/246946
+
 2019-06-28  Justin Michaud  <justin_michaud@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-07-01  Ryan Haddad  <ryanhaddad@apple.com>
+
+        Unreviewed, rolling out r246946.
+
+        Caused JSC test crashes on arm64
+
+        Reverted changeset:
+
+        "Add b3 macro lowering for CheckMul on arm64"
+        https://bugs.webkit.org/show_bug.cgi?id=199251
+        https://trac.webkit.org/changeset/246946
+
 2019-06-28  Justin Michaud  <justin_michaud@apple.com>
 

trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h

@@ -570 +570 @@
     }
 
-    void multiplySignExtend32(RegisterID left, RegisterID right, RegisterID dest)
-    {
-        m_assembler.smull(dest, left, right);
-    }
-
     void div32(RegisterID dividend, RegisterID divisor, RegisterID dest)
     {

trunk/Source/JavaScriptCore/assembler/testmasm.cpp

@@ -352 +352 @@
 }
 
-#if CPU(ARM64)
-void testMul32SignExtend()
-{
-    for (auto value : int32Operands()) {
-        auto mul = compile([=] (CCallHelpers& jit) {
-            jit.emitFunctionPrologue();
-
-            jit.multiplySignExtend32(GPRInfo::argumentGPR0, GPRInfo::argumentGPR1, GPRInfo::returnValueGPR);
-
-            jit.emitFunctionEpilogue();
-            jit.ret();
-        });
-
-        for (auto value2 : int32Operands())
-            CHECK_EQ(invoke<long int>(mul, value, value2), ((long int) value) * ((long int) value2));
-    }
-}
-#endif
-
 #if CPU(X86) || CPU(X86_64) || CPU(ARM64)
 void testCompareFloat(MacroAssembler::DoubleCondition condition)
@@ -1130 +1111 @@
     RUN(testMul32WithImmediates());
 
-#if CPU(ARM64)
-    RUN(testMul32SignExtend());
-#endif
-
 #if CPU(X86) || CPU(X86_64) || CPU(ARM64)
     RUN(testCompareFloat(MacroAssembler::DoubleEqual));

trunk/Source/JavaScriptCore/b3/B3LowerMacros.cpp

@@ -178 +178 @@
             }
 
-            case CheckMul: {
-                if (isARM64() && m_value->child(0)->type() == Int32) {
-                    CheckValue* checkMul = m_value->as<CheckValue>();
-
-                    Value* left = m_insertionSet.insert<Value>(m_index, SExt32, m_origin, m_value->child(0));
-                    Value* right = m_insertionSet.insert<Value>(m_index, SExt32, m_origin, m_value->child(1));
-                    Value* mulResult = m_insertionSet.insert<Value>(m_index, Mul, m_origin, left, right);
-                    Value* mulResult32 = m_insertionSet.insert<Value>(m_index, Trunc, m_origin, mulResult);
-                    Value* upperResult = m_insertionSet.insert<Value>(m_index, Trunc, m_origin,
-                        m_insertionSet.insert<Value>(m_index, SShr, m_origin, mulResult, m_insertionSet.insert<Const32Value>(m_index, m_origin, 32)));
-                    Value* signBit = m_insertionSet.insert<Value>(m_index, SShr, m_origin,
-                        mulResult32,
-                        m_insertionSet.insert<Const32Value>(m_index, m_origin, 31));
-                    Value* hasOverflowed = m_insertionSet.insert<Value>(m_index, NotEqual, m_origin, upperResult, signBit);
-
-                    CheckValue* check = m_insertionSet.insert<CheckValue>(m_index, Check, m_origin, hasOverflowed);
-                    check->setGenerator(checkMul->generator());
-                    check->clobberEarly(checkMul->earlyClobbered());
-                    check->clobberLate(checkMul->lateClobbered());
-                    check->append(checkMul->constrainedChild(0));
-                    check->append(checkMul->constrainedChild(1));
-
-                    m_value->replaceWithIdentity(mulResult32);
-                    m_changed = true;
-                }
-                break;
-            }
-
             case Switch: {
                 SwitchValue* switchValue = m_value->as<SwitchValue>();

trunk/Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -2603 +2603 @@
 
         case Mul: {
-            if (m_value->type() == Int64
-                && isValidForm(MultiplySignExtend32, Arg::Tmp, Arg::Tmp, Arg::Tmp)
-                && m_value->child(0)->opcode() == SExt32
-                && !m_locked.contains(m_value->child(0))) {
-                Value* opLeft = m_value->child(0);
-                Value* left = opLeft->child(0);
-                Value* opRight = m_value->child(1);
-                Value* right = nullptr;
-
-                if (opRight->opcode() == SExt32 && !m_locked.contains(opRight->child(0))) {
-                    right = opRight->child(0);
-                } else if (m_value->child(1)->isRepresentableAs<int32_t>() && !m_locked.contains(m_value->child(1))) {
-                    // We just use the 64-bit const int as a 32 bit const int directly
-                    right = opRight;
-                }
-
-                if (right) {
-                    append(MultiplySignExtend32, tmp(left), tmp(right), tmp(m_value));
-                    return;
-                }
-            }
             appendBinOp<Mul32, Mul64, MulDouble, MulFloat, Commutative>(
                 m_value->child(0), m_value->child(1));

trunk/Source/JavaScriptCore/b3/air/AirOpcode.opcodes

@@ -262 +262 @@
     Tmp, Tmp, Tmp
 
-arm64: MultiplySignExtend32 U:G:32, U:G:32, ZD:G:64
-    Tmp, Tmp, Tmp
-
 arm64: Div32 U:G:32, U:G:32, ZD:G:32
     Tmp, Tmp, Tmp

trunk/Source/JavaScriptCore/b3/testb3.cpp

@@ -1190 +1190 @@
 }
 
-void testMulArgs32SignExtend(int a, int b)
-{
-    Procedure proc;
-    if (proc.optLevel() < 1)
-        return;
-    BasicBlock* root = proc.addBlock();
-    Value* arg1 = root->appendNew<Value>(
-        proc, Trunc, Origin(),
-        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0));
-    Value* arg2 = root->appendNew<Value>(
-        proc, Trunc, Origin(),
-        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1));
-    Value* arg164 = root->appendNew<Value>(proc, SExt32, Origin(), arg1);
-    Value* arg264 = root->appendNew<Value>(proc, SExt32, Origin(), arg2);
-    Value* mul = root->appendNew<Value>(proc, Mul, Origin(), arg164, arg264);
-    root->appendNewControlValue(proc, Return, Origin(), mul);
-
-    auto code = compileProc(proc);
-
-    CHECK(invoke<long int>(*code, a, b) == ((long int) a) * ((long int) b));
-}
-
-void testMulImm32SignExtend(const int a, int b)
-{
-    Procedure proc;
-    if (proc.optLevel() < 1)
-        return;
-    BasicBlock* root = proc.addBlock();
-    Value* arg1 = root->appendNew<Const64Value>(proc, Origin(), a);
-    Value* arg2 = root->appendNew<Value>(
-        proc, Trunc, Origin(),
-        root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR1));
-    Value* arg264 = root->appendNew<Value>(proc, SExt32, Origin(), arg2);
-    Value* mul = root->appendNew<Value>(proc, Mul, Origin(), arg1, arg264);
-    root->appendNewControlValue(proc, Return, Origin(), mul);
-
-    auto code = compileProc(proc);
-
-    CHECK(invoke<long int>(*code, b) == ((long int) a) * ((long int) b));
-}
-
 void testMulLoadTwice()
 {
@@ -14677 +14636 @@
 {
     Procedure proc;
-
-    BasicBlock* root = proc.addBlock();
-
+    
+    BasicBlock* root = proc.addBlock();
+    
     root->appendNew<FenceValue>(proc, Origin());
     root->appendNew<Value>(proc, Return, Origin(), root->appendIntConstant(proc, Origin(), Int32, 42));
-
+    
     auto code = compileProc(proc);
     CHECK_EQ(invoke<int>(*code), 42);
@@ -14688 +14647 @@
         checkUsesInstruction(*code, "lock or $0x0, (%rsp)");
     if (isARM64())
-        checkUsesInstruction(*code, "dmb     ish");
+        checkUsesInstruction(*code, "dmb    ish");
     checkDoesNotUseInstruction(*code, "mfence");
-    checkDoesNotUseInstruction(*code, "dmb     ishst");
+    checkDoesNotUseInstruction(*code, "dmb    ishst");
 }
 
@@ -14696 +14655 @@
 {
     Procedure proc;
-
-    BasicBlock* root = proc.addBlock();
-
+    
+    BasicBlock* root = proc.addBlock();
+    
     root->appendNew<FenceValue>(proc, Origin(), HeapRange::top(), HeapRange());
     root->appendNew<Value>(proc, Return, Origin(), root->appendIntConstant(proc, Origin(), Int32, 42));
-
+    
     auto code = compileProc(proc);
     CHECK_EQ(invoke<int>(*code), 42);
@@ -14707 +14666 @@
     checkDoesNotUseInstruction(*code, "mfence");
     if (isARM64())
-        checkUsesInstruction(*code, "dmb     ishst");
+        checkUsesInstruction(*code, "dmb    ishst");
 }
 
@@ -14713 +14672 @@
 {
     Procedure proc;
-
-    BasicBlock* root = proc.addBlock();
-
+    
+    BasicBlock* root = proc.addBlock();
+    
     root->appendNew<FenceValue>(proc, Origin(), HeapRange(), HeapRange::top());
     root->appendNew<Value>(proc, Return, Origin(), root->appendIntConstant(proc, Origin(), Int32, 42));
-
+    
     auto code = compileProc(proc);
     CHECK_EQ(invoke<int>(*code), 42);
@@ -14724 +14683 @@
     checkDoesNotUseInstruction(*code, "mfence");
     if (isARM64())
-        checkUsesInstruction(*code, "dmb     ish");
-    checkDoesNotUseInstruction(*code, "dmb     ishst");
+        checkUsesInstruction(*code, "dmb    ish");
+    checkDoesNotUseInstruction(*code, "dmb    ishst");
 }
 
@@ -15003 +14962 @@
         CHECK_EQ(usesCSRs, !pin);
     };
-
+    
     go(true);
     go(false);
@@ -17199 +17158 @@
 
     auto shouldRun = [&] (const char* testName) -> bool {
-        // FIXME: These tests fail <https://bugs.webkit.org/show_bug.cgi?id=199330>.
-        if (!filter && isARM64()) {
-            for (auto& failingTest : {
-                "testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead",
-                "testNegFloatWithUselessDoubleConversion",
-                "testPinRegisters",
-            }) {
-                if (WTF::findIgnoringASCIICaseWithoutLength(testName, failingTest) != WTF::notFound) {
-                    dataLogLn("*** Warning: Skipping known-bad test: ", testName);
-                    return false;
-                }
-            }
-        }
-        if (!filter && isX86()) {
-            for (auto& failingTest : {
-                "testReportUsedRegistersLateUseFollowedByEarlyDefDoesNotMarkUseAsDead",
-            }) {
-                if (WTF::findIgnoringASCIICaseWithoutLength(testName, failingTest) != WTF::notFound) {
-                    dataLogLn("*** Warning: Skipping known-bad test: ", testName);
-                    return false;
-                }
-            }
-        }
         return !filter || WTF::findIgnoringASCIICaseWithoutLength(testName, filter) != WTF::notFound;
     };
@@ -17342 +17278 @@
     RUN(testMulImmArg(1, 0));
     RUN(testMulImmArg(3, 3));
-    RUN(testMulImm32SignExtend(1, 2));
-    RUN(testMulImm32SignExtend(0, 2));
-    RUN(testMulImm32SignExtend(1, 0));
-    RUN(testMulImm32SignExtend(3, 3));
-    RUN(testMulImm32SignExtend(0xFFFFFFFF, 0xFFFFFFFF));
-    RUN(testMulImm32SignExtend(0xFFFFFFFE, 0xFFFFFFFF));
-    RUN(testMulImm32SignExtend(0xFFFFFFFF, 0xFFFFFFFE));
     RUN(testMulArgs32(1, 1));
     RUN(testMulArgs32(1, 2));
-    RUN(testMulArgs32(0xFFFFFFFF, 0xFFFFFFFF));
-    RUN(testMulArgs32(0xFFFFFFFE, 0xFFFFFFFF));
-    RUN(testMulArgs32SignExtend(1, 1));
-    RUN(testMulArgs32SignExtend(1, 2));
-    RUN(testMulArgs32SignExtend(0xFFFFFFFF, 0xFFFFFFFF));
-    RUN(testMulArgs32SignExtend(0xFFFFFFFE, 0xFFFFFFFF));
     RUN(testMulLoadTwice());
     RUN(testMulAddArgsLeft());