Bug 161639 - MapHash should do constant folding when it has a constant argument and its legal to hash that value
Summary: MapHash should do constant folding when it has a constant argument and its le...
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:
Depends on:
Blocks:
 
Reported: 2016-09-06 12:14 PDT by Saam Barati
Modified: 2016-09-12 14:06 PDT (History)
16 users (show)

See Also:

Attachments
WIP (4.05 KB, patch)
2016-09-11 18:34 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (8.27 KB, patch)
2016-09-11 23:10 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (8.34 KB, patch)
2016-09-11 23:11 PDT, Saam Barati 		fpizlo: review+
Details | Formatted Diff | Diff
patch for landing (8.51 KB, patch)
2016-09-12 11:38 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2016-09-06 12:14:54 PDT 
....
Comment 1 Saam Barati 2016-09-11 18:34:05 PDT 
Created attachment 288544 [details]
WIP

I think this is the patch. Lets see what EWS thinks.
Comment 2 Saam Barati 2016-09-11 23:10:04 PDT 
Created attachment 288552 [details]
patch
Comment 3 Saam Barati 2016-09-11 23:11:29 PDT 
Created attachment 288554 [details]
patch
Comment 4 Filip Pizlo 2016-09-12 10:57:18 PDT 
Comment on attachment 288554 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:987
> +                // We don't want this JSValue to devolve into a double so
> +                // we cast it to an int32_t. When we actually use this
> +                // value, we will use it as if it were unsigned.
> +                setConstant(node, JSValue(static_cast<int32_t>(*hash)));

Does MapHash ordinarily return an int32?  If so, then this comment would be less confusing as: "Although C++ code uses uint32_t for the hash, the closest type in DFG IR is Int32 and that's what MapHash returns.  So, we have to cast to int32_t to avoid large unsigned values becoming doubles."

Also, I would use jsNumber(int32_t) instead of JSValue().
Comment 5 Saam Barati 2016-09-12 11:38:59 PDT 
Created attachment 288594 [details]
patch for landing
Comment 6 WebKit Commit Bot 2016-09-12 14:06:30 PDT 
Comment on attachment 288594 [details]
patch for landing

Clearing flags on attachment: 288594

Committed r205819: <http://trac.webkit.org/changeset/205819>
Comment 7 WebKit Commit Bot 2016-09-12 14:06:36 PDT 
All reviewed patches have been landed.  Closing bug.