Attachment 361095 Details for Bug 194231 – Patch

[patch] Patch

bug-194231-20190204135331.patch (text/plain), 3.08 KB, created by Robin Morisset on 2019-02-04 13:53:31 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Robin Morisset

Created: 2019-02-04 13:53:31 PST

Size: 3.08 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 240940)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2019-02-04  Robin Morisset  <rmorisset@apple.com>
>+
>+        when lowering AssertNotEmpty, create the value before creating the patchpoint
>+        https://bugs.webkit.org/show_bug.cgi?id=194231
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This test is painfully fragile: it tries to test that AssertNotEmpty on a constant produces valid B3 IR.
>+        The problem is that AssertNotEmpty is only created by DFGConstantFolding when it can simplify a CheckStructure, and constant folding is a bit capricious (https://bugs.webkit.org/show_bug.cgi?id=133947)
>+        So even tiny changes to this test can change the path code taken.
>+
>+        * stress/assert-not-empty.js: Added.
>+        (foo):
>+
> 2019-02-01  Mark Lam  <mark.lam@apple.com>
> 
>         Remove invalid assertion in DFG's compileDoubleRep().
>Index: JSTests/stress/assert-not-empty.js
>===================================================================
>--- JSTests/stress/assert-not-empty.js	(nonexistent)
>+++ JSTests/stress/assert-not-empty.js	(working copy)
>@@ -0,0 +1,12 @@
>+//@ runDefault("--useObjectAllocationSinking=0")
>+
>+function foo(o) {
>+  if (!o) {
>+    +eval;
>+  }
>+  o.x;
>+};
>+let i=0;
>+for (;i<100000;++i) {
>+  foo(Object);
>+}
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 240935)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-02-04  Robin Morisset  <rmorisset@apple.com>
>+
>+        when lowering AssertNotEmpty, create the value before creating the patchpoint
>+        https://bugs.webkit.org/show_bug.cgi?id=194231
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This is a very simple change: we should never generate B3 IR where an instruction depends on a value that comes later in the instruction stream.
>+        AssertNotEmpty was generating some such IR, it probably slipped through until now because it is a rather rare and tricky instruction to generate.
>+
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileAssertNotEmpty):
>+
> 2019-02-04  Mark Lam  <mark.lam@apple.com>
> 
>         DFG's doesGC() is incorrect about the SameValue node's behavior.
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(revision 240935)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(working copy)
>@@ -3111,8 +3111,9 @@ private:
>         if (!validationEnabled())
>             return;
> 
>+        LValue val = lowJSValue(m_node->child1());
>         PatchpointValue* patchpoint = m_out.patchpoint(Void);
>-        patchpoint->appendSomeRegister(lowJSValue(m_node->child1()));
>+        patchpoint->appendSomeRegister(val);
>         patchpoint->setGenerator(
>             [=] (CCallHelpers& jit, const StackmapGenerationParams& params) {
>                 AllowMacroScratchRegisterUsage allowScratch(jit);

Actions: View | Formatted Diff | Diff

Attachments on bug 194231: 361094 | 361095