Bug 202832 - Split ArithProfile into a Unary and a Binary version
Summary: Split ArithProfile into a Unary and a Binary version
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 202959 203752
Blocks: 193240
  Show dependency treegraph
 
Reported: 2019-10-10 17:18 PDT by Robin Morisset
Modified: 2019-11-18 15:24 PST (History)
12 users (show)

See Also:


Attachments
Patch (114.01 KB, patch)
2019-10-10 17:23 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (115.64 KB, patch)
2019-10-15 10:57 PDT, Robin Morisset
ews: commit-queue-
Details | Formatted Diff | Diff
Patch (115.64 KB, patch)
2019-10-15 14:50 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (122.01 KB, patch)
2019-10-21 15:00 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.47 KB, patch)
2019-10-22 16:34 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.41 KB, patch)
2019-10-23 18:54 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.39 KB, patch)
2019-10-29 16:20 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.44 KB, patch)
2019-10-29 16:58 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.25 KB, patch)
2019-10-30 11:38 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.73 KB, patch)
2019-10-30 13:32 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (121.18 KB, patch)
2019-10-30 14:07 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (120.70 KB, patch)
2019-11-02 22:28 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (121.53 KB, patch)
2019-11-07 15:07 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (122.10 KB, patch)
2019-11-07 17:57 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (130.92 KB, patch)
2019-11-12 13:47 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
diff (14.24 KB, patch)
2019-11-12 13:48 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (132.83 KB, patch)
2019-11-12 14:23 PST, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (132.83 KB, patch)
2019-11-12 14:31 PST, Robin Morisset
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (132.85 KB, patch)
2019-11-13 11:19 PST, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2019-10-10 17:18:45 PDT
ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
So I'd like to make a separate version that only has the data for a single argument, and thus takes half as much memory.
Comment 1 Robin Morisset 2019-10-10 17:23:02 PDT
Created attachment 380706 [details]
Patch
Comment 2 Keith Miller 2019-10-10 17:48:55 PDT
Comment on attachment 380706 [details]
Patch

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

r=me with question

> Source/JavaScriptCore/ChangeLog:9
> +        ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
> +        So in this patch I make a separate version that only has the data for a single argument, and thus takes half as much memory.

Did you see an improvement in RAMification?

> Source/JavaScriptCore/bytecode/ArithProfile.cpp:158
> +void printInternal(PrintStream& out, const UnaryArithProfile& profile)
> +{
> +    const char* separator = "";
> +
> +    out.print("Result:<");
> +    if (!profile.didObserveNonInt32()) {
> +        out.print("Int32");
> +        separator = "|";
> +    } else {
> +        if (profile.didObserveNegZeroDouble()) {
> +            out.print(separator, "NegZeroDouble");
> +            separator = "|";
> +        }
> +        if (profile.didObserveNonNegZeroDouble()) {
> +            out.print(separator, "NonNegZeroDouble");
> +            separator = "|";
> +        }
> +        if (profile.didObserveNonNumeric()) {
> +            out.print(separator, "NonNumeric");
> +            separator = "|";
> +        }
> +        if (profile.didObserveInt32Overflow()) {
> +            out.print(separator, "Int32Overflow");
> +            separator = "|";
> +        }
> +        if (profile.didObserveInt52Overflow()) {
> +            out.print(separator, "Int52Overflow");
> +            separator = "|";
> +        }
> +        if (profile.didObserveBigInt()) {
> +            out.print(separator, "BigInt");
> +            separator = "|";
> +        }
> +    }
> +    out.print(">");
> +
> +    out.print(" Arg ObservedType:<");
> +    out.print(profile.argObservedType());
> +    out.print(">");
> +
> +    out.print(" Arg ResultType:<");
> +    out.print(RawPointer(bitwise_cast<void*>(static_cast<uintptr_t>(profile.argResultType().bits()))));
> +    out.print(">");
> +}
> +

Seems like you could template this? e.g.

template<typename BitField>
inline void printInternal(PrintStream& out, const ArithProfile<BitField>&) ...

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:950
> +            if (!arithProfile)
> +                break;

Is this ever true? It kinda seems like it from reading the code below but that's somewhat surprising.
Comment 3 Robin Morisset 2019-10-14 11:48:06 PDT
(In reply to Keith Miller from comment #2)
> Comment on attachment 380706 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380706&action=review
> 
> r=me with question
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        ArithProfile was for a long time only used for add/sub/mul/div, but recently it started being used for negate. And it will soon also have to be used for inc and dec due to BigInt.
> > +        So in this patch I make a separate version that only has the data for a single argument, and thus takes half as much memory.
> 
> Did you see an improvement in RAMification?

I've not looked, but it would be super surprising considering that UnaryArithProfile is only used for OpNegate currently. Saving 2 bytes per OpNegate seems fairly negligible. It should matter more once I add it to OpInc and OpDec.

> 
> > Source/JavaScriptCore/bytecode/ArithProfile.cpp:158
> > +void printInternal(PrintStream& out, const UnaryArithProfile& profile)
> > +{
> > +    const char* separator = "";
> > +
> > +    out.print("Result:<");
> > +    if (!profile.didObserveNonInt32()) {
> > +        out.print("Int32");
> > +        separator = "|";
> > +    } else {
> > +        if (profile.didObserveNegZeroDouble()) {
> > +            out.print(separator, "NegZeroDouble");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveNonNegZeroDouble()) {
> > +            out.print(separator, "NonNegZeroDouble");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveNonNumeric()) {
> > +            out.print(separator, "NonNumeric");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveInt32Overflow()) {
> > +            out.print(separator, "Int32Overflow");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveInt52Overflow()) {
> > +            out.print(separator, "Int52Overflow");
> > +            separator = "|";
> > +        }
> > +        if (profile.didObserveBigInt()) {
> > +            out.print(separator, "BigInt");
> > +            separator = "|";
> > +        }
> > +    }
> > +    out.print(">");
> > +
> > +    out.print(" Arg ObservedType:<");
> > +    out.print(profile.argObservedType());
> > +    out.print(">");
> > +
> > +    out.print(" Arg ResultType:<");
> > +    out.print(RawPointer(bitwise_cast<void*>(static_cast<uintptr_t>(profile.argResultType().bits()))));
> > +    out.print(">");
> > +}
> > +
> 
> Seems like you could template this? e.g.
> 
> template<typename BitField>
> inline void printInternal(PrintStream& out, const ArithProfile<BitField>&)
> ...

I could probably factor some of the code, but I like having the different names LHS+RHS vs Arg. So the ObservedResult part is pretty much the only part which can be easily shared.

> 
> > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:950
> > +            if (!arithProfile)
> > +                break;
> 
> Is this ever true? It kinda seems like it from reading the code below but
> that's somewhat surprising.

I suspect one case where it can happen is if the ArithAdd comes from an OpInc. There may be some other cases.
Comment 4 WebKit Commit Bot 2019-10-14 13:28:49 PDT
Comment on attachment 380706 [details]
Patch

Clearing flags on attachment: 380706

Committed r251090: <https://trac.webkit.org/changeset/251090>
Comment 5 WebKit Commit Bot 2019-10-14 13:28:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 John Wilander 2019-10-14 15:28:52 PDT
This patch assert crashes on debug builds.
Comment 7 WebKit Commit Bot 2019-10-14 15:32:55 PDT
Re-opened since this is blocked by bug 202959
Comment 8 Radar WebKit Bug Importer 2019-10-14 15:33:07 PDT
<rdar://problem/56266847>
Comment 9 Radar WebKit Bug Importer 2019-10-14 15:33:08 PDT
<rdar://problem/56266848>
Comment 10 Robin Morisset 2019-10-15 10:57:50 PDT
Created attachment 381001 [details]
Patch

I'll be careful to wait for a green light from EWS before trying to land the patch this time.
Comment 11 Build Bot 2019-10-15 14:22:15 PDT
Comment on attachment 381001 [details]
Patch

Attachment 381001 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13135424

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
Comment 12 Robin Morisset 2019-10-15 14:50:47 PDT
Created attachment 381024 [details]
Patch

Trying again to check if the failing tests were related or not.
Comment 13 Robin Morisset 2019-10-21 14:31:23 PDT
I checked and this patch has no significant effect on RAMification.

Also I found a bug in it: I shrunk ArithProfile, but did not change the accesses to it in LowLevelAssembly{64/32_64}.asm.
Comment 14 Robin Morisset 2019-10-21 15:00:35 PDT
Created attachment 381463 [details]
Patch

Adding "orh" and using it instead of "ori" since UnaryArithProfile and BinaryArithProfile are now 16-bits.
Comment 15 Robin Morisset 2019-10-22 16:34:16 PDT
Created attachment 381626 [details]
Patch

rebased patch (it had some minor conflicts with Yusuke's recent patch).
Comment 16 Robin Morisset 2019-10-23 18:54:50 PDT
Created attachment 381763 [details]
Patch

rebasing, and removing a bit of dead code.
Comment 17 Robin Morisset 2019-10-29 16:20:07 PDT
Created attachment 382248 [details]
Patch

rebased
Comment 18 Robin Morisset 2019-10-29 16:58:52 PDT
Created attachment 382253 [details]
Patch

Fix iOS build
Comment 19 Robin Morisset 2019-10-30 11:38:48 PDT
Created attachment 382337 [details]
Patch

Fix another problem with orh on ARM.
Comment 20 Robin Morisset 2019-10-30 13:32:43 PDT
Created attachment 382346 [details]
Patch

Fix yet another problem with the offlineasm on ARM.
Comment 21 Robin Morisset 2019-10-30 14:07:38 PDT
Created attachment 382349 [details]
Patch

Copying my attempted fix from arm.rb to arm64.rb
Comment 22 Robin Morisset 2019-10-30 14:10:20 PDT
Comment on attachment 382349 [details]
Patch

Still hasn't fixed the problem..
Comment 23 Robin Morisset 2019-11-01 14:38:58 PDT
The problem came from https://bugs.webkit.org/show_bug.cgi?id=203752, I will rebase the patch and send it here for review once that bug is fixed.
Comment 24 Robin Morisset 2019-11-02 22:28:57 PDT
Created attachment 382693 [details]
Patch

It should build on ARM64, now that the bug with immediates and logical instructions has been fixed.
Comment 25 Build Bot 2019-11-03 01:11:44 PST
Comment on attachment 382693 [details]
Patch

Attachment 382693 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13206739

New failing tests:
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-dfg-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-no-ftl
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-baseline
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla-ftl-eager-no-cjit-validate-phases
mozilla-tests.yaml/ecma/Date/15.9.5.34-1.js.mozilla-llint
mozilla-tests.yaml/ecma/Date/15.9.5.14.js.mozilla
Comment 26 Keith Miller 2019-11-04 13:12:01 PST
Comment on attachment 382693 [details]
Patch

r=me.
Comment 27 Robin Morisset 2019-11-04 13:24:55 PST
Comment on attachment 382693 [details]
Patch

The bots failure related to Date are not caused by my patch, the bots just disliked the DST change somehow. So I am trying to land this.
Comment 28 WebKit Commit Bot 2019-11-04 14:20:45 PST
Comment on attachment 382693 [details]
Patch

Clearing flags on attachment: 382693

Committed r252015: <https://trac.webkit.org/changeset/252015>
Comment 29 WebKit Commit Bot 2019-11-04 14:20:47 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Truitt Savell 2019-11-04 15:23:00 PST
EWS did catch this build failure
Comment 32 Truitt Savell 2019-11-04 15:47:13 PST
Reverted r252015 for reason:

Broke the Windows build

Committed r252021: <https://trac.webkit.org/changeset/252021>
Comment 33 Robin Morisset 2019-11-07 15:07:40 PST
Created attachment 383078 [details]
Patch

fixed the cloop problem, will land again once all EWS bots are green.
Comment 34 Robin Morisset 2019-11-07 17:57:23 PST
Created attachment 383100 [details]
Patch

Fixing another cloop issue
Comment 35 Robin Morisset 2019-11-07 18:01:18 PST
Comment on attachment 383100 [details]
Patch

I checked the cloop build locally and it appears finally truly fixed. So trying to land this once again (not waiting for the windows bot because it appears hopelessly slow with 74 patches in wait).
Comment 36 WebKit Commit Bot 2019-11-07 18:46:17 PST
Comment on attachment 383100 [details]
Patch

Clearing flags on attachment: 383100

Committed r252229: <https://trac.webkit.org/changeset/252229>
Comment 37 WebKit Commit Bot 2019-11-07 18:46:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Matt Lewis 2019-11-08 16:11:35 PST
Reverted r252229 for reason:

This caused internal failures.

Committed r252273: <https://trac.webkit.org/changeset/252273>
Comment 39 Robin Morisset 2019-11-12 13:47:23 PST
Created attachment 383377 [details]
Patch

The problem that caused the roll-out was diagnosed by Mark, I still had some or32 left around in the ICs code, and when they were used on the now 16-bits ArithProfiles it could lead to metadata corruption.
Comment 40 Robin Morisset 2019-11-12 13:48:06 PST
Created attachment 383379 [details]
diff

Diff between last version of the patch (reviewed by Keith) and this one.
Comment 41 Robin Morisset 2019-11-12 13:51:03 PST
Comment on attachment 383377 [details]
Patch

Still visibly buggy on ARM
Comment 42 Robin Morisset 2019-11-12 14:23:36 PST
Created attachment 383382 [details]
Patch

Added the right load16/store16 to the ARM backends.
Comment 43 Robin Morisset 2019-11-12 14:29:39 PST
Comment on attachment 383382 [details]
Patch

Still failing on ARM..
Comment 44 Robin Morisset 2019-11-12 14:31:31 PST
Created attachment 383383 [details]
Patch

Just another stupid typo in MacroAssemblerARM64.h
Comment 45 Robin Morisset 2019-11-12 23:02:43 PST
Comment on attachment 383383 [details]
Patch

This patch breaks the MIPS port, I filed https://bugs.webkit.org/show_bug.cgi?id=204126 about it.
The windows bot is currently orange, as neither my patch nor ToT pass tests. Since jsc and Mac-debug-wk1 were in the same situation and have since turned green I suspect this was just a temporary breakage on ToT.
Comment 46 WebKit Commit Bot 2019-11-13 10:02:17 PST
Comment on attachment 383383 [details]
Patch

Rejecting attachment 383383 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 383383, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13247583
Comment 47 Robin Morisset 2019-11-13 11:19:45 PST
Created attachment 383473 [details]
Patch

Just fixed the Changelog.
Comment 48 Caio Lima 2019-11-13 11:33:59 PST
(In reply to Robin Morisset from comment #47)
> Created attachment 383473 [details]
> Patch
> 
> Just fixed the Changelog.

Hi Robin. I have a diff to fix the issue into MIPS. Don't know if it is too late to include this. I uploaded it into https://bugs.webkit.org/attachment.cgi?id=383476&action=review
Comment 49 WebKit Commit Bot 2019-11-13 12:07:37 PST
Comment on attachment 383473 [details]
Patch

Clearing flags on attachment: 383473

Committed r252422: <https://trac.webkit.org/changeset/252422>
Comment 50 WebKit Commit Bot 2019-11-13 12:07:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 51 Robin Morisset 2019-11-13 12:40:38 PST
(In reply to Caio Lima from comment #48)
> (In reply to Robin Morisset from comment #47)
> > Created attachment 383473 [details]
> > Patch
> > 
> > Just fixed the Changelog.
> 
> Hi Robin. I have a diff to fix the issue into MIPS. Don't know if it is too
> late to include this. I uploaded it into
> https://bugs.webkit.org/attachment.cgi?id=383476&action=review

Hi Caio,

Your patch does not apply cleanly on ToT. Feel free to rebase it, and I'll review it quickly so you can land it.