Changeset 267032 in webkit

Timestamp:

Sep 14, 2020 1:06:24 PM (4 years ago)

Author:

sbarati@apple.com

Message:

Remove bogus asserts in FTLLower that assume programs are compiled with sensible speculations
​https://bugs.webkit.org/show_bug.cgi?id=216485
<rdar://problem/68562804>

Reviewed by Keith Miller.

JSTests:

• stress/ftl-should-not-assume-speculations-are-sensible.js: Added.

(foo):

Source/JavaScriptCore:

We had an assert inside lowCell that if a value was not part of the JSValue
hashmap of values, then the type must not conform to being a cell. However,
consider a program like this:

`
x = ArithAdd(i32, i32) <-- x is an i32 here
if (b) {

Check(Cell:@x)
ArrayifyToStructure(@x, thingy)

}
<-- HERE
`

@x will live in FTLLower's i32 hashmap, but because of the AI rule for
ArrayifyToStructure, it will also have SpecCell in its type. This is totally
valid, and asserting that this isn't possible is wrong. (Obviously the above
speculation is stupid, as we will always exit at the Check, but it's valid IR.)

This patch removes this assertion from lowCell, and removes similar assertions
from other low* functions.

• ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::lowInt32):
(JSC::FTL::DFG::LowerDFGToB3::lowInt52):
(JSC::FTL::DFG::LowerDFGToB3::lowCell):
(JSC::FTL::DFG::LowerDFGToB3::lowBoolean):
(JSC::FTL::DFG::LowerDFGToB3::lowDouble):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/ftl-should-not-assume-speculations-are-sensible.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (5 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-09-14  Saam Barati  <sbarati@apple.com>
+
+        Remove bogus asserts in FTLLower that assume programs are compiled with sensible speculations
+        https://bugs.webkit.org/show_bug.cgi?id=216485
+        <rdar://problem/68562804>
+
+        Reviewed by Keith Miller.
+
+        * stress/ftl-should-not-assume-speculations-are-sensible.js: Added.
+        (foo):
+
 2020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-09-14  Saam Barati  <sbarati@apple.com>
+
+        Remove bogus asserts in FTLLower that assume programs are compiled with sensible speculations
+        https://bugs.webkit.org/show_bug.cgi?id=216485
+        <rdar://problem/68562804>
+
+        Reviewed by Keith Miller.
+
+        We had an assert inside lowCell that if a value was not part of the JSValue
+        hashmap of values, then the type must not conform to being a cell. However,
+        consider a program like this:
+        
+        ```
+        x = ArithAdd(i32, i32) <-- x is an i32 here
+        if (b) {
+            Check(Cell:@x)
+            ArrayifyToStructure(@x, thingy)
+        }
+        <-- HERE
+        ```
+        
+        @x will live in FTLLower's i32 hashmap, but because of the AI rule for
+        ArrayifyToStructure, it will also have SpecCell in its type. This is totally
+        valid, and asserting that this isn't possible is wrong. (Obviously the above
+        speculation is stupid, as we will always exit at the Check, but it's valid IR.)
+        
+        This patch removes this assertion from lowCell, and removes similar assertions
+        from other low* functions.
+
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::lowInt32):
+        (JSC::FTL::DFG::LowerDFGToB3::lowInt52):
+        (JSC::FTL::DFG::LowerDFGToB3::lowCell):
+        (JSC::FTL::DFG::LowerDFGToB3::lowBoolean):
+        (JSC::FTL::DFG::LowerDFGToB3::lowDouble):
+
 2020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -17016 +17016 @@
         }
 
-        DFG_ASSERT(m_graph, m_node, !(provenType(edge) & SpecInt32Only), provenType(edge));
         if (mayHaveTypeCheck(edge.useKind()))
             terminate(Uncountable);
@@ -17051 +17050 @@
         }
 
-        DFG_ASSERT(m_graph, m_node, !provenType(edge), provenType(edge));
         if (mayHaveTypeCheck(edge.useKind()))
             terminate(Uncountable);
@@ -17123 +17121 @@
         }
         
-        DFG_ASSERT(m_graph, m_node, !(provenType(edge) & SpecCellCheck), provenType(edge));
         if (mayHaveTypeCheck(edge.useKind()))
             terminate(Uncountable);
@@ -17284 +17281 @@
         }
 
-        DFG_ASSERT(m_graph, m_node, !(provenType(edge) & SpecBoolean), provenType(edge));
         if (mayHaveTypeCheck(edge.useKind()))
             terminate(Uncountable);
@@ -17297 +17293 @@
         if (isValid(value))
             return value.value();
-        DFG_ASSERT(m_graph, m_node, !provenType(edge), provenType(edge));
         if (mayHaveTypeCheck(edge.useKind()))
             terminate(Uncountable);