Changeset 209953 in webkit

Timestamp:

Dec 16, 2016 5:07:38 PM (7 years ago)

Author:

sbarati@apple.com

Message:

B3::DoubleToFloatReduction will accidentally convince itself it converted a Phi from Double to Float and then convert uses of that Phi into a use of FloatToDouble(@Phi)
​https://bugs.webkit.org/show_bug.cgi?id=165946

Reviewed by Keith Miller.

This was happening because the phase will convert some Phi nodes
from Double to Float. However, one place that did this conversion
forgot to first check if the Phi was already a Float. If it's already
a Float, a later part of the phase will be buggy if the phase claims that it has
converted it from Double->Float. The reason is that at the end of the
phase, we'll look for all uses of former Double Phi nodes and make them
be a use of ConvertFloatToDouble on the Phi, instead of a use of the Phi itself.
This is clearly wrong if the Phi were Float to begin with (and
therefore, the uses were Float uses to begin with).

• b3/B3ReduceDoubleToFloat.cpp:
• b3/testb3.cpp:

(JSC::B3::testReduceFloatToDoubleValidates):
(JSC::B3::run):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• b3/B3ReduceDoubleToFloat.cpp (modified) (2 diffs)
• b3/testb3.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-12-16  Saam Barati  <sbarati@apple.com>
+
+        B3::DoubleToFloatReduction will accidentally convince itself it converted a Phi from Double to Float and then convert uses of that Phi into a use of FloatToDouble(@Phi)
+        https://bugs.webkit.org/show_bug.cgi?id=165946
+
+        Reviewed by Keith Miller.
+
+        This was happening because the phase will convert some Phi nodes
+        from Double to Float. However, one place that did this conversion
+        forgot to first check if the Phi was already a Float. If it's already
+        a Float, a later part of the phase will be buggy if the phase claims that it has
+        converted it from Double->Float. The reason is that at the end of the
+        phase, we'll look for all uses of former Double Phi nodes and make them
+        be a use of ConvertFloatToDouble on the Phi, instead of a use of the Phi itself.
+        This is clearly wrong if the Phi were Float to begin with (and
+        therefore, the uses were Float uses to begin with).
+
+        * b3/B3ReduceDoubleToFloat.cpp:
+        * b3/testb3.cpp:
+        (JSC::B3::testReduceFloatToDoubleValidates):
+        (JSC::B3::run):
+
 2016-12-16  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp

@@ -232 +232 @@
 
         if (value->opcode() == Phi) {
-            convertPhi(value);
+            ASSERT(value->type() == Double || value->type() == Float);
+            if (value->type() == Double)
+                convertPhi(value);
             return value;
         }
@@ -242 +244 @@
     {
         ASSERT(phi->opcode() == Phi);
+        ASSERT(phi->type() == Double);
         phi->setType(Float);
         m_convertedPhis.add(phi);

trunk/Source/JavaScriptCore/b3/testb3.cpp

@@ -4534 +4534 @@
     CHECK(isIdentical(invoke<float>(*code, 1, bitwise_cast<int32_t>(value)), value + 42.5f));
     CHECK(isIdentical(invoke<float>(*code, 0, bitwise_cast<int32_t>(value)), static_cast<float>(M_PI)));
+}
+
+void testReduceFloatToDoubleValidates()
+{
+    // Simple case of:
+    //     f = DoubleToFloat(Bitcast(argGPR0))
+    //     if (a) {
+    //         x = FloatConst()
+    //     else
+    //         x = FloatConst()
+    //     p = Phi(x)
+    //     a = Mul(p, p)
+    //     b = Add(a, f)
+    //     c = Add(p, b)
+    //     Return(c)
+    //
+    // This should not crash in the validator after ReduceFloatToDouble.
+    Procedure proc;
+    BasicBlock* root = proc.addBlock();
+    BasicBlock* thenCase = proc.addBlock();
+    BasicBlock* elseCase = proc.addBlock();
+    BasicBlock* tail = proc.addBlock();
+
+    Value* condition = root->appendNew<ArgumentRegValue>(proc, Origin(), GPRInfo::argumentGPR0);
+    Value* thingy = root->appendNew<Value>(proc, BitwiseCast, Origin(), condition);
+    thingy = root->appendNew<Value>(proc, DoubleToFloat, Origin(), thingy); // Make the phase think it has work to do.
+    root->appendNewControlValue(
+        proc, Branch, Origin(),
+        condition,
+        FrequentedBlock(thenCase), FrequentedBlock(elseCase));
+
+    UpsilonValue* thenValue = thenCase->appendNew<UpsilonValue>(proc, Origin(),
+        thenCase->appendNew<ConstFloatValue>(proc, Origin(), 11.5));
+    thenCase->appendNewControlValue(proc, Jump, Origin(), FrequentedBlock(tail));
+
+    UpsilonValue* elseValue = elseCase->appendNew<UpsilonValue>(proc, Origin(), 
+        elseCase->appendNew<ConstFloatValue>(proc, Origin(), 10.5));
+    elseCase->appendNewControlValue(proc, Jump, Origin(), FrequentedBlock(tail));
+
+    Value* phi =  tail->appendNew<Value>(proc, Phi, Float, Origin());
+    thenValue->setPhi(phi);
+    elseValue->setPhi(phi);
+    Value* result = tail->appendNew<Value>(proc, Mul, Origin(), 
+            phi, phi);
+    result = tail->appendNew<Value>(proc, Add, Origin(), 
+            result,
+            thingy);
+    result = tail->appendNew<Value>(proc, Add, Origin(), 
+            phi,
+            result);
+    tail->appendNewControlValue(proc, Return, Origin(), result);
+
+    auto code = compile(proc);
+    CHECK(isIdentical(invoke<float>(*code, 1), 11.5f * 11.5f + static_cast<float>(bitwise_cast<double>(static_cast<uint64_t>(1))) + 11.5f));
+    CHECK(isIdentical(invoke<float>(*code, 0), 10.5f * 10.5f + static_cast<float>(bitwise_cast<double>(static_cast<uint64_t>(0))) + 10.5f));
 }
 
@@ -14629 +14684 @@
     RUN_BINARY(testCompareFloatToDoubleThroughPhi, floatingPointOperands<float>(), floatingPointOperands<float>());
     RUN_UNARY(testDoubleToFloatThroughPhi, floatingPointOperands<float>());
+    RUN(testReduceFloatToDoubleValidates());
     RUN_UNARY(testDoubleProducerPhiToFloatConversion, floatingPointOperands<float>());
     RUN_UNARY(testDoubleProducerPhiToFloatConversionWithDoubleConsumer, floatingPointOperands<float>());