Bug 198453 - Reenable Gigacage on ARM64.
Summary: Reenable Gigacage on ARM64.
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on: 198486 198698
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-01 10:43 PDT by Keith Miller
Modified: 2019-06-16 20:46 PDT (History)
13 users (show)

See Also:


Attachments
Patch (11.04 KB, patch)
2019-06-01 10:47 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (31.72 KB, patch)
2019-06-06 05:19 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (31.46 KB, patch)
2019-06-06 05:27 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (31.42 KB, patch)
2019-06-06 05:39 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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