Let's do this to 1. pave the way to handle mandelbrot.js in FTL 2. support ArrayStorage / SlowPutArrayStorage arrays, like, tagged template call site object (SixSpeed)
Created attachment 310383 [details] Patch WIP: it works! Need tests
Comment on attachment 310383 [details] Patch Attachment 310383 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3762326 New failing tests: jsc-layout-tests.yaml/js/script-tests/array-iterate-backwards.js.layout-ftl-eager-no-cjit
Created attachment 310388 [details] Patch WIP
Created attachment 310390 [details] Patch WIP
Comment on attachment 310390 [details] Patch Attachment 310390 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3764007 New failing tests: http/tests/media/hls/video-controls-live-stream.html
Created attachment 310422 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 310768 [details] Patch
Comment on attachment 310768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review LGTM, just want clarification on your use of a particular operation. > Source/JavaScriptCore/ChangeLog:12 > + This patch adds GetByVal support for ArrayStorage and SlowPutArrayStorage. > + To lower CheckInBounds in FTL, we add a new GetVectorLength op. It only accepts > + ArrayStorage and SlowPutArrayStorage, then it produces vector length. > + CheckInBounds uses this vector length to perform bound checking for ArrayStorage > + and SlowPutArrayStorage. Do we need something like this for the DFG as well, or is that already done? > Source/JavaScriptCore/dfg/DFGClobberize.h:1143 > + case GetVectorLength: { You should add a validation rule for GetVectorLength > Source/JavaScriptCore/ftl/FTLCapabilities.cpp:351 > + default: > + return CannotCompile; Isn't this invalid form? > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:3601 > + vmCall(Int64, m_out.operation(operationGetByValArrayInt), m_callFrame, base, index)); How do we know the base is an array? What if it's just a normal object w/ this kind of indexing type? Maybe this bug already exists in the FTL today?
Maybe we already have a bug. This program: ``` function foo(a, i) { return a[i]; } noInline(foo); let array = {}; array[0] = 10; array[1] = 20; array.foo = 20; for (let i = 0; i < 10000; ++i) { foo(array, (Math.random() * 3) | 0); } ``` Produces this FTL code for `foo`: ``` 19:<!0:-> ExitOK(MustGen, W:SideState, bc#0) 23:< 3:-> GetStack(JS|PureInt, Final, arg1, machine:arg1, FlushedCell, R:Stack(6), bc#0) 24:<!0:-> CheckArray(Cell:@23, MustGen, Int32+NonArray+OutOfBounds+AsIs, R:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoType, Exits, bc#0) 21:< 1:-> GetStack(JS|PureInt, BoolInt32, arg2, machine:arg2, FlushedInt32, R:Stack(7), bc#0) 15:<!0:-> KillStack(MustGen, loc0, W:SideState, ClobbersExit, bc#0) 4:<!0:-> ZombieHint(MustGen, loc0, W:SideState, ClobbersExit, bc#0, ExitInvalid) 13:<!0:-> KillStack(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid) 6:<!0:-> ZombieHint(MustGen, loc1, W:SideState, ClobbersExit, bc#0, ExitInvalid) 11:<!0:-> KillStack(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid) 8:<!0:-> ZombieHint(MustGen, loc2, W:SideState, ClobbersExit, bc#0, ExitInvalid) 9:<!0:-> KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid) 10:<!0:-> ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#0, ExitInvalid) 7:<!0:-> KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid) 12:<!0:-> ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#0, ExitInvalid) 5:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid) 14:<!0:-> ZombieHint(MustGen, loc5, W:SideState, ClobbersExit, bc#0, ExitInvalid) 2:<!0:-> KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#1) 18:<!0:-> ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#1, ExitInvalid) 1:<!0:-> KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#3) 20:<!0:-> ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#3, ExitInvalid) 22:<!0:-> CheckTraps(MustGen, W:Watchpoint_fire, Exits, ClobbersExit, bc#6) 29:<!0:-> InvalidationPoint(MustGen, W:SideState, Exits, bc#7) 30:< 1:-> GetButterfly(Cell:@23, Storage|PureInt, R:JSObject_butterfly, Exits, bc#7) 25:<!2:-> GetByVal(KnownCell:Kill:@23, Int32:Kill:@21, Untyped:Kill:@30, JS|MustGen|UseAsOther, Int32, Int32+NonArray+OutOfBounds+AsIs, R:World, W:Heap, Exits, ClobbersExit, bc#7) predicting NonBoolInt32 0:<!0:-> KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#7, ExitInvalid) 26:<!0:-> MovHint(Untyped:@25, MustGen, loc6, W:SideState, ClobbersExit, bc#7, ExitInvalid) 27:<!0:-> InvalidationPoint(MustGen, W:SideState, Exits, bc#7, exit: bc#13) 28:<!0:-> Return(Untyped:Kill:@25, MustGen, W:SideState, Exits, bc#13) ``` Digging into the CheckArray code, I don't think it guarantees the speculation fails if the thing is not an array. It's more like check indexing shape. My worry i the GetByVal code calls operationGetByValArrayInt even when we haven't proven it's an array. Let me verify this actually happens, I'm building now.
(In reply to Saam Barati from comment #9) > Maybe we already have a bug. This program: > > ``` > function foo(a, i) { > return a[i]; > } > noInline(foo); > > let array = {}; > array[0] = 10; > array[1] = 20; > array.foo = 20; > > for (let i = 0; i < 10000; ++i) { > foo(array, (Math.random() * 3) | 0); > } > ``` > > Produces this FTL code for `foo`: > ``` > 19:<!0:-> ExitOK(MustGen, W:SideState, bc#0) > 23:< 3:-> GetStack(JS|PureInt, Final, arg1, machine:arg1, FlushedCell, > R:Stack(6), bc#0) > 24:<!0:-> CheckArray(Cell:@23, MustGen, Int32+NonArray+OutOfBounds+AsIs, > R:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoType, Exits, bc#0) > 21:< 1:-> GetStack(JS|PureInt, BoolInt32, arg2, machine:arg2, > FlushedInt32, R:Stack(7), bc#0) > 15:<!0:-> KillStack(MustGen, loc0, W:SideState, ClobbersExit, bc#0) > 4:<!0:-> ZombieHint(MustGen, loc0, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 13:<!0:-> KillStack(MustGen, loc1, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 6:<!0:-> ZombieHint(MustGen, loc1, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 11:<!0:-> KillStack(MustGen, loc2, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 8:<!0:-> ZombieHint(MustGen, loc2, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 9:<!0:-> KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 10:<!0:-> ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 7:<!0:-> KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 12:<!0:-> ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 5:<!0:-> KillStack(MustGen, loc5, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 14:<!0:-> ZombieHint(MustGen, loc5, W:SideState, ClobbersExit, bc#0, > ExitInvalid) > 2:<!0:-> KillStack(MustGen, loc3, W:SideState, ClobbersExit, bc#1) > 18:<!0:-> ZombieHint(MustGen, loc3, W:SideState, ClobbersExit, bc#1, > ExitInvalid) > 1:<!0:-> KillStack(MustGen, loc4, W:SideState, ClobbersExit, bc#3) > 20:<!0:-> ZombieHint(MustGen, loc4, W:SideState, ClobbersExit, bc#3, > ExitInvalid) > 22:<!0:-> CheckTraps(MustGen, W:Watchpoint_fire, Exits, ClobbersExit, bc#6) > 29:<!0:-> InvalidationPoint(MustGen, W:SideState, Exits, bc#7) > 30:< 1:-> GetButterfly(Cell:@23, Storage|PureInt, R:JSObject_butterfly, > Exits, bc#7) > 25:<!2:-> GetByVal(KnownCell:Kill:@23, Int32:Kill:@21, Untyped:Kill:@30, > JS|MustGen|UseAsOther, Int32, Int32+NonArray+OutOfBounds+AsIs, R:World, > W:Heap, Exits, ClobbersExit, bc#7) predicting NonBoolInt32 > 0:<!0:-> KillStack(MustGen, loc6, W:SideState, ClobbersExit, bc#7, > ExitInvalid) > 26:<!0:-> MovHint(Untyped:@25, MustGen, loc6, W:SideState, ClobbersExit, > bc#7, ExitInvalid) > 27:<!0:-> InvalidationPoint(MustGen, W:SideState, Exits, bc#7, exit: bc#13) > 28:<!0:-> Return(Untyped:Kill:@25, MustGen, W:SideState, Exits, bc#13) > ``` > > Digging into the CheckArray code, I don't think it guarantees the > speculation fails if the thing is not an array. It's more like check > indexing shape. My worry i the GetByVal code calls operationGetByValArrayInt > even when we haven't proven it's an array. Let me verify this actually > happens, I'm building now. Yeah we totally call this function here. Anyways, this is a benign bug, since it immediately lowers the type to cell: ``` ALWAYS_INLINE EncodedJSValue getByValCellInt(ExecState* exec, JSCell* base, int32_t index) { VM* vm = &exec->vm(); NativeCallFrameTracer tracer(vm, exec); if (index < 0) { // Go the slowest way possible becase negative indices don't use indexed storage. return JSValue::encode(JSValue(base).get(exec, Identifier::from(exec, index))); } // Use this since we know that the value is out of bounds. return JSValue::encode(JSValue(base).get(exec, static_cast<unsigned>(index))); } EncodedJSValue JIT_OPERATION operationGetByValArrayInt(ExecState* exec, JSArray* base, int32_t index) { return getByValCellInt(exec, base, index); } ``` That said, lets please rename this function since it's clearly being misused. Perhaps we can just pass an object base.
r=me with function rename and type modification.
Comment on attachment 310768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review Thanks. OK, I'll rename the function to operationGetByValObjectInt. >> Source/JavaScriptCore/ChangeLog:12 >> + and SlowPutArrayStorage. > > Do we need something like this for the DFG as well, or is that already done? Yeah, DFG already has this. FTL missed it, then this patch adds it.
Comment on attachment 310768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310768&action=review >> Source/JavaScriptCore/dfg/DFGClobberize.h:1143 >> + case GetVectorLength: { > > You should add a validation rule for GetVectorLength It's nice idea. Added. >> Source/JavaScriptCore/ftl/FTLCapabilities.cpp:351 >> + return CannotCompile; > > Isn't this invalid form? Yes, I've changed it to RELEASE_ASSERT_NOT_REACHED().
Committed r217202: <http://trac.webkit.org/changeset/217202>