Bug 198453

Summary: Reenable Gigacage on ARM64.
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: JavaScriptCoreAssignee: Keith Miller <keith_miller>
Status: REOPENED ---    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, dean_johnson, ews-watchlist, fpizlo, mark.lam, msaboff, pvollan, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 198486, 198698    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Keith Miller 2019-06-01 10:43:38 PDT
Reenable Gigacage on ARM64.
Comment 1 Keith Miller 2019-06-01 10:47:57 PDT
Created attachment 371113 [details]
Patch
Comment 2 Keith Miller 2019-06-01 10:49:58 PDT
Comment on attachment 371113 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371113&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        Gigacaging would otherwise strip a PAC failed authenticate bit we
> +        force a load of the pointer into some garbage register.

I was thinking this would be free because we would load the length from the base of the pointer anyway. Unfortunately, that is only true for butterflies and not for TypedArray storage. I'm not sure what the right fix is... perhaps this is still fine but it's probably better to not pollute the caching hierarchy...
Comment 3 WebKit Commit Bot 2019-06-02 13:02:06 PDT
Comment on attachment 371113 [details]
Patch

Clearing flags on attachment: 371113

Committed r246022: <https://trac.webkit.org/changeset/246022>
Comment 4 WebKit Commit Bot 2019-06-02 13:02:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-06-02 13:03:40 PDT
<rdar://problem/51340879>
Comment 6 WebKit Commit Bot 2019-06-03 09:45:03 PDT
Re-opened since this is blocked by bug 198486
Comment 7 Dean Johnson 2019-06-03 14:44:35 PDT
Note: Internal tests also show a 3-5x magnitude performance regression on arm64e, compared to arm64. Probably a bug there.
Comment 8 Saam Barati 2019-06-05 13:24:55 PDT
Gonna take this over since Keith is busy at the moment with standards meetings.
Comment 9 Keith Miller 2019-06-06 05:19:54 PDT
Created attachment 371487 [details]
Patch
Comment 10 Keith Miller 2019-06-06 05:22:32 PDT
(In reply to Saam Barati from comment #8)
> Gonna take this over since Keith is busy at the moment with standards
> meetings.

But I have a patch!
Comment 11 Keith Miller 2019-06-06 05:27:00 PDT
Created attachment 371488 [details]
Patch
Comment 12 Michael Saboff 2019-06-06 05:38:11 PDT
Comment on attachment 371488 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371488&action=review

r=me with one comment.

> Source/JavaScriptCore/offlineasm/instructions.rb:276
> +     "bfiq", # Bit field insert <source reg> <width immediate> <last bit written> <dest reg>

The order of width and last bit written seem to be backwards.
Comment 13 Keith Miller 2019-06-06 05:39:16 PDT
Created attachment 371490 [details]
Patch for landing
Comment 14 WebKit Commit Bot 2019-06-06 06:21:03 PDT
Comment on attachment 371490 [details]
Patch for landing

Clearing flags on attachment: 371490

Committed r246150: <https://trac.webkit.org/changeset/246150>
Comment 15 WebKit Commit Bot 2019-06-06 06:21:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Saam Barati 2019-06-06 08:41:54 PDT
Comment on attachment 371490 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=371490&action=review

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6846
> +    m_jit.cageWithoutUntaging(Gigacage::JSValue, dataGPR);

“Untaging” => “Untagging”
Comment 17 Saam Barati 2019-06-06 08:42:15 PDT
(In reply to Keith Miller from comment #10)
> (In reply to Saam Barati from comment #8)
> > Gonna take this over since Keith is busy at the moment with standards
> > meetings.
> 
> But I have a patch!

👍🏼
Comment 18 Saam Barati 2019-06-06 08:42:41 PDT
What was the perf regression from the prior patch?
Comment 19 Keith Miller 2019-06-06 11:48:43 PDT
(In reply to Saam Barati from comment #18)
> What was the perf regression from the prior patch?

I was forcing a load of the untagged PC before passing it off to the gigacage stripping code.
Comment 20 Keith Miller 2019-06-06 11:51:04 PDT
Comment on attachment 371490 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=371490&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6846
>> +    m_jit.cageWithoutUntaging(Gigacage::JSValue, dataGPR);
> 
> “Untaging” => “Untagging”

I uploaded a patch to fix the typo:  https://bugs.webkit.org/show_bug.cgi?id=198617
Comment 21 WebKit Commit Bot 2019-06-09 13:24:40 PDT
Re-opened since this is blocked by bug 198698