Changeset 249075 in webkit


Ignore:
Timestamp:
Aug 23, 2019 4:08:06 PM (5 years ago)
Author:
Tadeu Zagallo
Message:

Remove MaximalFlushInsertionPhase
​https://bugs.webkit.org/show_bug.cgi?id=201036

Reviewed by Saam Barati.

JSTests:

Remove all the references to maximal flush

  • stress/arith-ceil-on-various-types.js:

(checkCompileCountForUselessNegativeZero):

  • stress/arith-floor-on-various-types.js:

(checkCompileCountForUselessNegativeZero):

  • stress/arith-negate-on-various-types.js:

(checkCompileCountForUselessNegativeZero):

  • stress/arith-round-on-various-types.js:

(checkCompileCountForUselessNegativeZero):

  • stress/arith-trunc-on-various-types.js:

(checkCompileCountForUselessNegativeZero):

  • stress/dfg-compare-eq-via-nonSpeculativeNonPeepholeCompareNullOrUndefined.js:
  • stress/has-indexed-property-should-accept-non-int32.js:
  • stress/has-indexed-property-with-worsening-array-mode.js:
  • stress/known-int32-cant-be-used-across-bytecode-boundary.js:
  • stress/read-dead-bytecode-locals-in-must-handle-values1.js:
  • stress/read-dead-bytecode-locals-in-must-handle-values2.js:
  • stress/rest-parameter-many-arguments.js:
  • stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js:
  • stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js:
  • stress/to-index-string-should-not-assume-incoming-value-is-uint32.js:

Source/JavaScriptCore:

Maximal flush has found too many false positives recently, so we decided it's finally time
to remove it instead of hacking it to fix the most recent false positive.

The most recent false positive was caused by a LoadVarargs followed by a SetArgumentDefinitely
for the argument count that was being flushed in a much later block. Now, since that block was
the head of a loop, and there was a SetLocal in the same block to the same variable, this
generated a Phi of both values, which then led to the unification of their VariableAccessData
in the unification phase. This caused AI to assign the Int52 type to argument count, which
broke the AI’s assumption that it should always be an Int32.

  • JavaScriptCore.xcodeproj/project.pbxproj:
  • Sources.txt:
  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::handleVarargsInlining):

  • dfg/DFGMaximalFlushInsertionPhase.cpp: Removed.
  • dfg/DFGMaximalFlushInsertionPhase.h: Removed.
  • dfg/DFGPlan.cpp:

(JSC::DFG::Plan::compileInThreadImpl):

  • runtime/Options.cpp:

(JSC::recomputeDependentOptions):

  • runtime/Options.h:
Location:
trunk
Files:
2 deleted
23 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r249069 r249075  
     12019-08-23  Tadeu Zagallo  <tzagallo@apple.com>
     2
     3        Remove MaximalFlushInsertionPhase
     4        https://bugs.webkit.org/show_bug.cgi?id=201036
     5
     6        Reviewed by Saam Barati.
     7
     8        Remove all the references to maximal flush
     9
     10        * stress/arith-ceil-on-various-types.js:
     11        (checkCompileCountForUselessNegativeZero):
     12        * stress/arith-floor-on-various-types.js:
     13        (checkCompileCountForUselessNegativeZero):
     14        * stress/arith-negate-on-various-types.js:
     15        (checkCompileCountForUselessNegativeZero):
     16        * stress/arith-round-on-various-types.js:
     17        (checkCompileCountForUselessNegativeZero):
     18        * stress/arith-trunc-on-various-types.js:
     19        (checkCompileCountForUselessNegativeZero):
     20        * stress/dfg-compare-eq-via-nonSpeculativeNonPeepholeCompareNullOrUndefined.js:
     21        * stress/has-indexed-property-should-accept-non-int32.js:
     22        * stress/has-indexed-property-with-worsening-array-mode.js:
     23        * stress/known-int32-cant-be-used-across-bytecode-boundary.js:
     24        * stress/read-dead-bytecode-locals-in-must-handle-values1.js:
     25        * stress/read-dead-bytecode-locals-in-must-handle-values2.js:
     26        * stress/rest-parameter-many-arguments.js:
     27        * stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js:
     28        * stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js:
     29        * stress/to-index-string-should-not-assume-incoming-value-is-uint32.js:
     30
    1312019-08-23  Justin Michaud  <justin_michaud@apple.com>
    232

  • trunk/JSTests/stress/arith-ceil-on-various-types.js

    r228513 r249075  
    141141function checkCompileCountForUselessNegativeZero(testFunction)
    142142{
    143     if (jscOptions().useMaximalFlushInsertionPhase) {
    144         // If we forced a flush after the operation, the negative zero becomes
    145         // observable and we may be overly optimistic.
    146         return numberOfDFGCompiles(testFunction) <= 2;
    147     }
    148143    return numberOfDFGCompiles(testFunction) <= 1;
    149144}

  • trunk/JSTests/stress/arith-floor-on-various-types.js

    r228513 r249075  
    141141function checkCompileCountForUselessNegativeZero(testFunction)
    142142{
    143     if (jscOptions().useMaximalFlushInsertionPhase) {
    144         // If we forced a flush after the operation, the negative zero becomes
    145         // observable and we may be overly optimistic.
    146         return numberOfDFGCompiles(testFunction) <= 2;
    147     }
    148143    return numberOfDFGCompiles(testFunction) <= 1;
    149144}

  • trunk/JSTests/stress/arith-negate-on-various-types.js

    r228513 r249075  
    121121function checkCompileCountForUselessNegativeZero(testFunction)
    122122{
    123     if (jscOptions().useMaximalFlushInsertionPhase) {
    124         // If we forced a flush after the operation, the negative zero becomes
    125         // observable and we may be overly optimistic.
    126         return numberOfDFGCompiles(testFunction) <= 2;
    127     }
    128123    return numberOfDFGCompiles(testFunction) <= 1;
    129124}

  • trunk/JSTests/stress/arith-round-on-various-types.js

    r228513 r249075  
    141141function checkCompileCountForUselessNegativeZero(testFunction)
    142142{
    143     if (jscOptions().useMaximalFlushInsertionPhase) {
    144         // If we forced a flush after the operation, the negative zero becomes
    145         // observable and we may be overly optimistic.
    146         return numberOfDFGCompiles(testFunction) <= 2;
    147     }
    148143    return numberOfDFGCompiles(testFunction) <= 1;
    149144}

  • trunk/JSTests/stress/arith-trunc-on-various-types.js

    r228513 r249075  
    141141function checkCompileCountForUselessNegativeZero(testFunction)
    142142{
    143     if (jscOptions().useMaximalFlushInsertionPhase) {
    144         // If we forced a flush after the operation, the negative zero becomes
    145         // observable and we may be overly optimistic.
    146         return numberOfDFGCompiles(testFunction) <= 2;
    147     }
    148143    return numberOfDFGCompiles(testFunction) <= 1;
    149144}

  • trunk/JSTests/stress/dfg-compare-eq-via-nonSpeculativeNonPeepholeCompareNullOrUndefined.js

    r243344 r249075  
    1 //@ runDefault("--collectContinuously=true", "--collectContinuouslyPeriodMS=0.15", "--useMaximalFlushInsertionPhase=true", "--useLLInt=false", "--useFTLJIT=false", "--jitPolicyScale=0")
     1//@ runDefault("--collectContinuously=true", "--collectContinuouslyPeriodMS=0.15", "--useLLInt=false", "--useFTLJIT=false", "--jitPolicyScale=0")
    22
    33// This test exercises DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined().

  • trunk/JSTests/stress/has-indexed-property-should-accept-non-int32.js

    r244211 r249075  
    1 //@ runDefault("--useRandomizingFuzzerAgent=1", "--jitPolicyScale=0", "--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
     1//@ runDefault("--useRandomizingFuzzerAgent=1", "--jitPolicyScale=0", "--useConcurrentJIT=0")
    22function foo(obj) {
    33  for (var x in obj) {

  • trunk/JSTests/stress/has-indexed-property-with-worsening-array-mode.js

    r241968 r249075  
    1 //@ requireOptions("--watchdog=1000", "--watchdog-exception-ok", "--useMaximalFlushInsertionPhase=1")
     1//@ requireOptions("--watchdog=1000", "--watchdog-exception-ok")
    22// This test only seems to reproduce the issue when it runs in an infinite loop. So we use the watchdog to time it out.
    33for (let x in [0]) {

  • trunk/JSTests/stress/known-int32-cant-be-used-across-bytecode-boundary.js

    r242954 r249075  
    1 //@ runDefault("--useConcurrentJIT=0", "--useMaximalFlushInsertionPhase=1")
     1//@ runDefault("--useConcurrentJIT=0")
    22
    33function foo() {

  • trunk/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values1.js

    r242192 r249075  
    1 //@ runDefault("--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
     1//@ runDefault("--useConcurrentJIT=0")
    22function bar(x) {
    33  if (x) {

  • trunk/JSTests/stress/read-dead-bytecode-locals-in-must-handle-values2.js

    r242280 r249075  
    1 //@ runDefault("--useMaximalFlushInsertionPhase=1", "--useConcurrentJIT=0")
     1//@ runDefault("--useConcurrentJIT=0")
    22function bar(c) {
    33    if (c > 1) {

  • trunk/JSTests/stress/rest-parameter-many-arguments.js

    r237919 r249075  
    11//@ skip if $architecture == "x86"
    2 //@ if $architecture == "x86" then defaultSpotCheckNoMaximalFlush else defaultRun end
    32
    43function assert(b) {

  • trunk/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js

    r245341 r249075  
    1 //@ runDefault("--useMaximalFlushInsertionPhase=1", "--jitPolicyScale=0", "--useConcurrentJIT=0")
     1//@ runDefault("--jitPolicyScale=0", "--useConcurrentJIT=0")
    22
    33function f0() {

  • trunk/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js

    r245341 r249075  
    1 //@ runDefault("--useMaximalFlushInsertionPhase=1", "--validateGraphAtEachPhase=1")
     1//@ runDefault("--validateGraphAtEachPhase=1")
    22
    33function f1() {

  • trunk/JSTests/stress/to-index-string-should-not-assume-incoming-value-is-uint32.js

    r244057 r249075  
    1 //@ runDefault("--useMaximalFlushInsertionPhase=1", "--useRandomizingFuzzerAgent=1")
     1//@ runDefault("--useRandomizingFuzzerAgent=1")
    22
    33function foo() {

  • trunk/Source/JavaScriptCore/ChangeLog

    r249073 r249075  
     12019-08-23  Tadeu Zagallo  <tzagallo@apple.com>
     2
     3        Remove MaximalFlushInsertionPhase
     4        https://bugs.webkit.org/show_bug.cgi?id=201036
     5
     6        Reviewed by Saam Barati.
     7
     8        Maximal flush has found too many false positives recently, so we decided it's finally time
     9        to remove it instead of hacking it to fix the most recent false positive.
     10
     11        The most recent false positive was caused by a LoadVarargs followed by a SetArgumentDefinitely
     12        for the argument count that was being flushed in a much later block. Now, since that block was
     13        the head of a loop, and there was a SetLocal in the same block to the same variable, this
     14        generated a Phi of both values, which then led to the unification of their VariableAccessData
     15        in the unification phase. This caused AI to assign the Int52 type to argument count, which
     16        broke the AI’s assumption that it should always be an Int32.
     17
     18        * JavaScriptCore.xcodeproj/project.pbxproj:
     19        * Sources.txt:
     20        * dfg/DFGByteCodeParser.cpp:
     21        (JSC::DFG::ByteCodeParser::handleVarargsInlining):
     22        * dfg/DFGMaximalFlushInsertionPhase.cpp: Removed.
     23        * dfg/DFGMaximalFlushInsertionPhase.h: Removed.
     24        * dfg/DFGPlan.cpp:
     25        (JSC::DFG::Plan::compileInThreadImpl):
     26        * runtime/Options.cpp:
     27        (JSC::recomputeDependentOptions):
     28        * runtime/Options.h:
     29
    1302019-08-23  Ross Kirsling  <ross.kirsling@sony.com>
    231

  • trunk/Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj

    r248938 r249075  
    12161216                79EE0C001B4AFB85000385C9 /* VariableEnvironment.h in Headers */ = {isa = PBXBuildFile; fileRef = 79EE0BFE1B4AFB85000385C9 /* VariableEnvironment.h */; settings = {ATTRIBUTES = (Private, ); }; };
    12171217                79EFD4841EBC045C00F3DFEA /* JSWebAssemblyCodeBlockHeapCellType.h in Headers */ = {isa = PBXBuildFile; fileRef = 79EFD4821EBC045C00F3DFEA /* JSWebAssemblyCodeBlockHeapCellType.h */; settings = {ATTRIBUTES = (Private, ); }; };
    1218                 79F8FC1F1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.h in Headers */ = {isa = PBXBuildFile; fileRef = 79F8FC1D1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.h */; };
    12191218                79FC8A081E32E9F000D88F0E /* DFGRegisteredStructure.h in Headers */ = {isa = PBXBuildFile; fileRef = 79FC8A071E32E9F000D88F0E /* DFGRegisteredStructure.h */; settings = {ATTRIBUTES = (Private, ); }; };
    12201219                7A9774A8206B82E4008D03D0 /* JSWeakValue.h in Headers */ = {isa = PBXBuildFile; fileRef = 7A9774A7206B82C9008D03D0 /* JSWeakValue.h */; settings = {ATTRIBUTES = (Private, ); }; };
    … …  
    39913990                79EFD4811EBC045C00F3DFEA /* JSWebAssemblyCodeBlockHeapCellType.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = JSWebAssemblyCodeBlockHeapCellType.cpp; path = js/JSWebAssemblyCodeBlockHeapCellType.cpp; sourceTree = "<group>"; };
    39923991                79EFD4821EBC045C00F3DFEA /* JSWebAssemblyCodeBlockHeapCellType.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = JSWebAssemblyCodeBlockHeapCellType.h; path = js/JSWebAssemblyCodeBlockHeapCellType.h; sourceTree = "<group>"; };
    3993                 79F8FC1C1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; name = DFGMaximalFlushInsertionPhase.cpp; path = dfg/DFGMaximalFlushInsertionPhase.cpp; sourceTree = "<group>"; };
    3994                 79F8FC1D1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DFGMaximalFlushInsertionPhase.h; path = dfg/DFGMaximalFlushInsertionPhase.h; sourceTree = "<group>"; };
    39953992                79FC8A071E32E9F000D88F0E /* DFGRegisteredStructure.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DFGRegisteredStructure.h; path = dfg/DFGRegisteredStructure.h; sourceTree = "<group>"; };
    39963993                7A9774A6206B828C008D03D0 /* JSWeakValue.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = JSWeakValue.cpp; sourceTree = "<group>"; };
    … …  
    77827779                                A767B5B317A0B9650063D940 /* DFGLoopPreHeaderCreationPhase.cpp */,
    77837780                                A767B5B417A0B9650063D940 /* DFGLoopPreHeaderCreationPhase.h */,
    7784                                 79F8FC1C1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.cpp */,
    7785                                 79F8FC1D1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.h */,
    77867781                                0F5874EB194FEB1200AAB2C1 /* DFGMayExit.cpp */,
    77877782                                0F5874EC194FEB1200AAB2C1 /* DFGMayExit.h */,
    … …  
    91689163                                A7D89CFC17A0B8CC00773AD8 /* DFGLivenessAnalysisPhase.h in Headers */,
    91699164                                A767B5B617A0B9650063D940 /* DFGLoopPreHeaderCreationPhase.h in Headers */,
    9170                                 79F8FC1F1B9FED0F00CA66AB /* DFGMaximalFlushInsertionPhase.h in Headers */,
    91719165                                0F5874EE194FEB1200AAB2C1 /* DFGMayExit.h in Headers */,
    91729166                                0F2BDC451522801B00CD8910 /* DFGMinifiedGraph.h in Headers */,

  • trunk/Source/JavaScriptCore/Sources.txt

    r248938 r249075  
    365365dfg/DFGLivenessAnalysisPhase.cpp
    366366dfg/DFGLoopPreHeaderCreationPhase.cpp
    367 dfg/DFGMaximalFlushInsertionPhase.cpp
    368367dfg/DFGMayExit.cpp
    369368dfg/DFGMinifiedGraph.cpp

  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r248906 r249075  
    18631863    registerOffset = -WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -registerOffset);
    18641864
    1865     Vector<VirtualRegister> setArgumentMaybes;
    1866    
    18671865    auto insertChecks = [&] (CodeBlock* codeBlock) {
    18681866        emitFunctionChecks(callVariant, callTargetNode, thisArgument);
    … …  
    19291927           
    19301928            Node* setArgument = addToGraph(numSetArguments >= mandatoryMinimum ? SetArgumentMaybe : SetArgumentDefinitely, OpInfo(variable));
    1931             if (numSetArguments >= mandatoryMinimum && Options::useMaximalFlushInsertionPhase())
    1932                 setArgumentMaybes.append(variable->local());
    19331929            m_currentBlock->variablesAtTail.setOperand(variable->local(), setArgument);
    19341930            ++numSetArguments;
    … …  
    19451941    inlineCall(callTargetNode, result, callVariant, registerOffset, maxNumArguments, kind, nullptr, insertChecks);
    19461942
    1947     for (VirtualRegister reg : setArgumentMaybes)
    1948         setDirect(reg, jsConstant(jsUndefined()), ImmediateNakedSet);
    19491943
    19501944    VERBOSE_LOG("Successful inlining (varargs, monomorphic).\nStack: ", currentCodeOrigin(), "\n");

  • trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

    r248846 r249075  
    5252#include "DFGLivenessAnalysisPhase.h"
    5353#include "DFGLoopPreHeaderCreationPhase.h"
    54 #include "DFGMaximalFlushInsertionPhase.h"
    5554#include "DFGMovHintRemovalPhase.h"
    5655#include "DFGOSRAvailabilityAnalysisPhase.h"
    … …  
    286285    RUN_PHASE(performLiveCatchVariablePreservationPhase);
    287286
    288     if (Options::useMaximalFlushInsertionPhase())
    289         RUN_PHASE(performMaximalFlushInsertion);
    290    
    291287    RUN_PHASE(performCPSRethreading);
    292288    RUN_PHASE(performUnification);

  • trunk/Source/JavaScriptCore/runtime/Options.cpp

    r249013 r249075  
    472472        Options::useConcurrentJIT() = false;
    473473    }
    474     if (Options::useMaximalFlushInsertionPhase()) {
    475         Options::useOSREntryToDFG() = false;
    476         Options::useOSREntryToFTL() = false;
    477     }
    478    
    479474#if ENABLE(SEPARATED_WX_HEAP)
    480475    // Override globally for now. Longer term we'll just make the default

  • trunk/Source/JavaScriptCore/runtime/Options.h

    r248919 r249075  
    311311    v(unsigned, maximumVarargsForInlining, 100, Normal, nullptr) \
    312312    \
    313     v(bool, useMaximalFlushInsertionPhase, false, Normal, "Setting to true allows the DFG's MaximalFlushInsertionPhase to run.") \
    314     \
    315313    v(unsigned, maximumBinaryStringSwitchCaseLength, 50, Normal, nullptr) \
    316314    v(unsigned, maximumBinaryStringSwitchTotalLength, 2000, Normal, nullptr) \
    … …  
    546544    v(enablePolyvariantCallInlining, usePolyvariantCallInlining, SameOption) \
    547545    v(enablePolyvariantByIdInlining, usePolyvariantByIdInlining, SameOption) \
    548     v(enableMaximalFlushInsertionPhase, useMaximalFlushInsertionPhase, SameOption) \
    549546    v(objectsAreImmortal, useImmortalObjects, SameOption) \
    550547    v(showObjectStatistics, dumpObjectStatistics, SameOption) \
Note: See TracChangeset for help on using the changeset viewer.