Bug 158911 - InlineAccess should do StringLength
Summary: InlineAccess should do StringLength
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-06-19 12:51 PDT by Saam Barati
Modified: 2018-08-31 10:07 PDT (History)
14 users (show)

See Also:


Attachments
patch (14.29 KB, patch)
2016-06-20 12:56 PDT, Saam Barati
keith_miller: review+
Details | Formatted Diff | Diff
patch (14.05 KB, patch)
2018-08-29 18:08 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (14.05 KB, patch)
2018-08-29 18:49 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-06-19 12:51:02 PDT
...
Comment 1 Saam Barati 2016-06-20 12:56:09 PDT
Created attachment 281668 [details]
patch
Comment 2 Keith Miller 2016-06-20 13:09:05 PDT
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.
Comment 3 Yusuke Suzuki 2018-08-03 18:17:47 PDT
Are you planning landing this patch? I think InlinedAccessCase for StringLength is profitable.
Comment 4 Saam Barati 2018-08-03 18:46:46 PDT
(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.
Comment 5 Saam Barati 2018-08-28 23:15:46 PDT
Will try to get this landed tomorrow.
Comment 6 Saam Barati 2018-08-29 18:08:55 PDT
Created attachment 348456 [details]
patch
Comment 7 EWS Watchlist 2018-08-29 18:10:27 PDT
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.
Comment 8 Saam Barati 2018-08-29 18:49:26 PDT
Created attachment 348460 [details]
patch
Comment 9 Yusuke Suzuki 2018-08-30 11:21:42 PDT
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 10 Saam Barati 2018-08-30 11:38:38 PDT
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.
Comment 11 Saam Barati 2018-08-30 11:39:55 PDT
(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 12 Mark Lam 2018-08-30 12:06:25 PDT
Comment on attachment 348460 [details]
patch

FWIW, r=me too.
Comment 13 WebKit Commit Bot 2018-08-30 12:47:06 PDT
Comment on attachment 348460 [details]
patch

Clearing flags on attachment: 348460

Committed r235517: <https://trac.webkit.org/changeset/235517>
Comment 14 WebKit Commit Bot 2018-08-30 12:47:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Philippe Normand 2018-08-31 04:49:13 PDT
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
Comment 16 Radar WebKit Bug Importer 2018-08-31 04:49:23 PDT
<rdar://problem/43935465>
Comment 17 Philippe Normand 2018-08-31 05:06:16 PDT
The JSCOnly ARMv7 build bots also expose this issue.
Comment 18 Radar WebKit Bug Importer 2018-08-31 05:06:31 PDT
<rdar://problem/43935775>
Comment 19 Mark Lam 2018-08-31 09:55:38 PDT
<rdar://problem/43935465>
Comment 20 Mark Lam 2018-08-31 10:07:30 PDT
ARMv7 speculative build fix landed in r235557: <https://trac.webkit.org/r235557>.