...
Created attachment 281668 [details] patch
Comment on attachment 281668 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=281668&action=review r=me with comments. > Source/JavaScriptCore/bytecode/InlineAccess.cpp:305 > + auto branchToSlowPath = jit.patchableBranch8( > + CCallHelpers::NotEqual, > + CCallHelpers::Address(base, JSCell::typeInfoTypeOffset()), > + CCallHelpers::TrustedImm32(StringType)); I think it would be helpful here to put a comment saying the the patchableBranch is used to avoid branch compaction.
Are you planning landing this patch? I think InlinedAccessCase for StringLength is profitable.
(In reply to Yusuke Suzuki from comment #3) > Are you planning landing this patch? I think InlinedAccessCase for > StringLength is profitable. I forgot about this! I'll rebase and land it soon.
Will try to get this landed tomorrow.
Created attachment 348456 [details] patch
Attachment 348456 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/InlineAccess.cpp:51: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] ERROR: Source/JavaScriptCore/bytecode/InlineAccess.cpp:52: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348460 [details] patch
Comment on attachment 348460 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=348460&action=review r=me > Source/JavaScriptCore/bytecode/InlineAccess.cpp:173 > + GPRReg base = stubInfo.baseGPR(); Nice improvement. > Source/JavaScriptCore/bytecode/StructureStubInfo.h:201 > #endif I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now!
Comment on attachment 348460 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=348460&action=review >> Source/JavaScriptCore/bytecode/StructureStubInfo.h:201 >> #endif > > I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now! Let's do this as a follow up for all these fields.
(In reply to Saam Barati from comment #10) > Comment on attachment 348460 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348460&action=review > > >> Source/JavaScriptCore/bytecode/StructureStubInfo.h:201 > >> #endif > > > > I think we can use GPRReg directly here (instead of int8_t), since they are `enum : int8_t` now! > > Let's do this as a follow up for all these fields. https://bugs.webkit.org/show_bug.cgi?id=189166
Comment on attachment 348460 [details] patch FWIW, r=me too.
Comment on attachment 348460 [details] patch Clearing flags on attachment: 348460 Committed r235517: <https://trac.webkit.org/changeset/235517>
All reviewed patches have been landed. Closing bug.
This patch broke my ARMv7 linux WPE build: Source/JavaScriptCore/bytecode/InlineAccess.cpp:53:13: error: 'class JSC::CCallHelpers' has no member named 'patchableBranch8'; did you mean 'patchableBranch32'? jit.patchableBranch8( ^~~~~~~~~~~~~~~~ patchableBranch32 In file included from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource29.cpp:5:0: Source/JavaScriptCore/bytecode/InlineAccess.cpp: In static member function 'static bool JSC::InlineAccess::generateStringLength(JSC::StructureStubInfo&)': /home/phil/workspace/sources/wpewebkit/Source/JavaScriptCore/bytecode/InlineAccess.cpp:298:33: error: 'class JSC::CCallHelpers' has no member named 'patchableBranch8'; did you mean 'patchableBranch32'? auto branchToSlowPath = jit.patchableBranch8( ^~~~~~~~~~~~~~~~ patchableBranch32
<rdar://problem/43935465>
The JSCOnly ARMv7 build bots also expose this issue.
<rdar://problem/43935775>
ARMv7 speculative build fix landed in r235557: <https://trac.webkit.org/r235557>.