Add for-in OwnStructureMode optimizations to LLInt
Created attachment 435422 [details] Patch
Created attachment 435425 [details] Patch
Created attachment 435427 [details] Patch
Comment on attachment 435427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435427&action=review drive by comment. > Source/JavaScriptCore/runtime/PropertyOffset.h:56 > UNUSED_PARAM(offset); > ASSERT(offset >= invalidOffset); nit: Might as well make this: ASSERT_UNUSED(offset, offset >= invalidOffset);
<rdar://problem/82126099>
Created attachment 436213 [details] Patch
Created attachment 436217 [details] Patch
Created attachment 436219 [details] Patch
Comment on attachment 435427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435427&action=review >> Source/JavaScriptCore/runtime/PropertyOffset.h:56 >> ASSERT(offset >= invalidOffset); > > nit: Might as well make this: > ASSERT_UNUSED(offset, offset >= invalidOffset); I could but maybe not worth changing it unnecessarily for `git blame`
Comment on attachment 436219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436219&action=review > Source/JavaScriptCore/ChangeLog:9 > + This patch adds the optimizations we have for OwnStructureMode in > + the Baseline to the LLInt. The patch also adds redundant self move worth saying what those optimizations are right here. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3097 > + # FIXME: This will always call the slow path on at least the first/last execution of EnumeratorNext for any given loop. Why is that the case? I don't follow. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3114 > + storeVariable(get, m_index, t2, t3) how does this work? Index isn't tagged here. Seems sketchy that it's a value in the stack. In baseline, you're adding using 64 bit numbers, which seems more legit, and then zexting before using this as an index when accessing the property names array. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3131 > + # FIXME: This should be orb but that doesn't exist for some reason... file a bug and link it. > Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3149 > + sxi2q t2, t2 zxi2q seems semantically more correct. > Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:99 > + # Windows complains about signed integers being cast to unsigned but we just want the bits. > + outp.puts "\#if COMPILER(MSVC)" > + outp.puts "\#pragma warning(disable:4308)" > + outp.puts "\#endif" is this needed?
Comment on attachment 436219 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436219&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3097 >> + # FIXME: This will always call the slow path on at least the first/last execution of EnumeratorNext for any given loop. > > Why is that the case? I don't follow. Because the first time mode will be InitMode so we'll go to the slow path and the last time because we'll have to call the slow path to figure out if there's GenericMode properties. >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3114 >> + storeVariable(get, m_index, t2, t3) > > how does this work? Index isn't tagged here. Seems sketchy that it's a value in the stack. In baseline, you're adding using 64 bit numbers, which seems more legit, and then zexting before using this as an index when accessing the property names array. Yeah, that seems like a bug. I guess no one looks at the high bits of the value? I'll change the above addi to addq. >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3131 >> + # FIXME: This should be orb but that doesn't exist for some reason... > > file a bug and link it. Done. >> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3149 >> + sxi2q t2, t2 > > zxi2q seems semantically more correct. Yeah, I think the max inline property capacity is pretty small but I'll change it. >> Source/JavaScriptCore/offlineasm/generate_offset_extractor.rb:99 >> + outp.puts "\#endif" > > is this needed? Yeah, MSVC complains that we try to multiply a signed and unsigned value, which is a really dumb warning... It's also why I had to cast the sizeof(EncodedJSValue) for the first out of line offset computation.
Created attachment 436300 [details] Patch
Comment on attachment 436300 [details] Patch r=me
Committed r281523 (240896@main): <https://commits.webkit.org/240896@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436300 [details].
Hi, this broke the cloop build. (I guess we don't have any cloop EWS.) [1/127] Generating ../../JavaScriptCore/DerivedSources/LLIntAssembly.h FAILED: JavaScriptCore/DerivedSources/LLIntAssembly.h cd /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources && /usr/bin/cmake -E env CMAKE_CXX_COMPILER_ID=GNU GCC_OFFLINEASM_SOURCE_MAP=OFF /usr/bin/ruby /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb -I/home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/ /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/bin/LLIntOffsetsExtractor /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/LLIntAssembly.h normal && /usr/bin/cmake -E touch_nocreate /home/mcatanzaro/Projects/WebKit/WebKitBuild/JSCOnly/JavaScriptCore/DerivedSources/LLIntAssembly.h /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:652:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 @sourceFile=#<SourceFile:0x000056431b01dfb0 @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, @value=32> (due to /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:3111) (LoweringError) from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:134:in `lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:192:in `block in lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `each' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:413:in `block (5 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:107:in `inAsm' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:412:in `block (4 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:410:in `block (3 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:397:in `block (2 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `each' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `block in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `open' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `<main>' /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb:652:in `lowerC_LOOP': undefined method `uint32MemRef' for #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 @sourceFile=#<SourceFile:0x000056431b01dfb0 @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, @value=32> (NoMethodError) from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:134:in `lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:192:in `block in lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `each' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/backends.rb:190:in `lower' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:413:in `block (5 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:107:in `inAsm' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:412:in `block (4 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:210:in `emitCodeInConfiguration' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:410:in `block (3 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/settings.rb:107:in `forSettings' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:397:in `block (2 levels) in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `each' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:393:in `block in <main>' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `open' from /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/asm.rb:384:in `<main>' ninja: build stopped: subcommand failed.
Re-opened since this is blocked by bug 229494
(In reply to Michael Catanzaro from comment #15) > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb: > 652:in `lowerC_LOOP': undefined method `uint32MemRef' for > #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 > @sourceFile=#<SourceFile:0x000056431b01dfb0 > @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/ > llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, > @value=32> (due to > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/ > LowLevelInterpreter64.asm:3111) (LoweringError) I don't understand any of this code, so I decided to do a rollout for now, sorry. Does the error message make sense to you? I don't see uint32MemRef anywhere in the diff, but I've double-checked and the error is definitely caused by this commit.
(In reply to Michael Catanzaro from comment #17) > (In reply to Michael Catanzaro from comment #15) > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb: > > 652:in `lowerC_LOOP': undefined method `uint32MemRef' for > > #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 > > @sourceFile=#<SourceFile:0x000056431b01dfb0 > > @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/ > > llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, > > @value=32> (due to > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/ > > LowLevelInterpreter64.asm:3111) (LoweringError) > > I don't understand any of this code, so I decided to do a rollout for now, > sorry. Does the error message make sense to you? I don't see uint32MemRef > anywhere in the diff, but I've double-checked and the error is definitely > caused by this commit. It's because the code in the LowLevelInterpreter*.asm is fed to a script which turns it into an inline assembly C++ file. I see the issue though. I'm a bit surprised it didn't cause any breakage though O.o
(In reply to Keith Miller from comment #18) > (In reply to Michael Catanzaro from comment #17) > > (In reply to Michael Catanzaro from comment #15) > > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/offlineasm/cloop.rb: > > > 652:in `lowerC_LOOP': undefined method `uint32MemRef' for > > > #<Immediate:0x000056431ed48a20 @codeOrigin=#<CodeOrigin:0x000056431b395aa8 > > > @sourceFile=#<SourceFile:0x000056431b01dfb0 > > > @name=#<Pathname:/home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/ > > > llint/LowLevelInterpreter64.asm>, @fileNumber=3>, @lineNumber=3111>, > > > @value=32> (due to > > > /home/mcatanzaro/Projects/WebKit/Source/JavaScriptCore/llint/ > > > LowLevelInterpreter64.asm:3111) (LoweringError) > > > > I don't understand any of this code, so I decided to do a rollout for now, > > sorry. Does the error message make sense to you? I don't see uint32MemRef > > anywhere in the diff, but I've double-checked and the error is definitely > > caused by this commit. > > It's because the code in the LowLevelInterpreter*.asm is fed to a script > which turns it into an inline assembly C++ file. I see the issue though. I'm > a bit surprised it didn't cause any breakage though O.o Welp, figured it out. It's because all the code in the enumerator_next fast path wasn't running. I was comparing the stack address of m_mode with 2 which will always be false and take the slow path. Whoops.
Created attachment 436395 [details] Patch for landing
Committed r281565 (240930@main): <https://commits.webkit.org/240930@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436395 [details].