B3::DoubleToFloatReduction will accidentally convince itself it converted a Phi from...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Dec 2016 01:07:38 +0000 (01:07 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 17 Dec 2016 01:07:38 +0000 (01:07 +0000)
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):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@209953 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/B3ReduceDoubleToFloat.cpp
Source/JavaScriptCore/b3/testb3.cpp

index 96cff4b..a3a9c1a 100644 (file)
@@ -1,3 +1,25 @@
+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>
 
         De-duplicate finally blocks.
index 8ece389..ef92811 100644 (file)
@@ -231,7 +231,9 @@ private:
             return insertionSet.insert<ConstFloatValue>(valueIndex, value->origin(), static_cast<float>(value->asDouble()));
 
         if (value->opcode() == Phi) {
-            convertPhi(value);
+            ASSERT(value->type() == Double || value->type() == Float);
+            if (value->type() == Double)
+                convertPhi(value);
             return value;
         }
         RELEASE_ASSERT_NOT_REACHED();
@@ -241,6 +243,7 @@ private:
     void convertPhi(Value* phi)
     {
         ASSERT(phi->opcode() == Phi);
+        ASSERT(phi->type() == Double);
         phi->setType(Float);
         m_convertedPhis.add(phi);
     }
index 6ffbfe5..b76a985 100644 (file)
@@ -4535,6 +4535,61 @@ void testDoubleToFloatThroughPhi(float value)
     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));
+}
+
 void testDoubleProducerPhiToFloatConversion(float value)
 {
     Procedure proc;
@@ -14628,6 +14683,7 @@ void run(const char* filter)
     RUN_BINARY(testCompareOneFloatToDouble, floatingPointOperands<float>(), floatingPointOperands<double>());
     RUN_BINARY(testCompareFloatToDoubleThroughPhi, floatingPointOperands<float>(), floatingPointOperands<float>());
     RUN_UNARY(testDoubleToFloatThroughPhi, floatingPointOperands<float>());
+    RUN(testReduceFloatToDoubleValidates());
     RUN_UNARY(testDoubleProducerPhiToFloatConversion, floatingPointOperands<float>());
     RUN_UNARY(testDoubleProducerPhiToFloatConversionWithDoubleConsumer, floatingPointOperands<float>());
     RUN_BINARY(testDoubleProducerPhiWithNonFloatConst, floatingPointOperands<float>(), floatingPointOperands<double>());