Bug 193957 - mul32 should convert powers of 2 to an lshift
Summary: mul32 should convert powers of 2 to an lshift
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-28 22:51 PST by Keith Miller
Modified: 2019-01-30 14:43 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.51 KB, patch)
2019-01-28 22:52 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (539.41 KB, application/zip)
2019-01-28 23:46 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (501.28 KB, application/zip)
2019-01-28 23:55 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (413.55 KB, application/zip)
2019-01-29 00:01 PST, EWS Watchlist
no flags Details
Patch for landing (1.51 KB, patch)
2019-01-29 00:02 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (3.30 KB, patch)
2019-01-30 13:49 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (3.30 KB, patch)
2019-01-30 13:49 PST, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (3.44 KB, patch)
2019-01-30 14:00 PST, 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-01-28 22:51:07 PST
mul32 should convert powers of 2 to an lshift
Comment 1 Keith Miller 2019-01-28 22:52:14 PST
Created attachment 360441 [details]
Patch
Comment 2 Yusuke Suzuki 2019-01-28 22:59:22 PST
Comment on attachment 360441 [details]
Patch

r=me
Comment 3 Mark Lam 2019-01-28 23:00:15 PST
Comment on attachment 360441 [details]
Patch

Please add a test to verify that the operation is computing the right values.  testmasm is probably where you can do this easily.
Comment 4 EWS Watchlist 2019-01-28 23:46:47 PST
Comment on attachment 360441 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 5 EWS Watchlist 2019-01-28 23:46:48 PST
Created attachment 360446 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 6 EWS Watchlist 2019-01-28 23:55:50 PST
Comment on attachment 360441 [details]
Patch

Attachment 360441 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10934977

Number of test failures exceeded the failure limit.
Comment 7 EWS Watchlist 2019-01-28 23:55:52 PST
Created attachment 360447 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 8 Keith Miller 2019-01-29 00:00:36 PST
(In reply to Mark Lam from comment #3)
> Comment on attachment 360441 [details]
> Patch
> 
> Please add a test to verify that the operation is computing the right
> values.  testmasm is probably where you can do this easily.

Does the fact that I was off by 1 and every JIT test failed count? :P
Comment 9 EWS Watchlist 2019-01-29 00:01:46 PST
Comment on attachment 360441 [details]
Patch

Attachment 360441 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10934948

Number of test failures exceeded the failure limit.
Comment 10 EWS Watchlist 2019-01-29 00:01:50 PST
Created attachment 360448 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 11 Keith Miller 2019-01-29 00:02:36 PST
Created attachment 360449 [details]
Patch for landing
Comment 12 Mark Lam 2019-01-29 00:06:21 PST
Comment on attachment 360449 [details]
Patch for landing

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

> Source/JavaScriptCore/assembler/MacroAssembler.h:1910
> +            lshift32(src, TrustedImm32(getLSBSet(imm.m_value)), dest);

I see that you've fixed the issue with getLSBSet(imm.m_value).  You should also check if imm.m_value == 1, and if so, move(src, dest) instead.  Again, please add some mul32 tests to testmasm.
Comment 13 Mark Lam 2019-01-29 00:13:21 PST
(In reply to Keith Miller from comment #8)
> (In reply to Mark Lam from comment #3)
> > Comment on attachment 360441 [details]
> > Patch
> > 
> > Please add a test to verify that the operation is computing the right
> > values.  testmasm is probably where you can do this easily.
> 
> Does the fact that I was off by 1 and every JIT test failed count? :P

I suppose.  However, for masm changes like this, I still think it's good practice to have the specific edge cases for mul32 in testmasm to point out where we go wrong.  This is better than having to debug what went wrong in a test that exercises a lot more code.
Comment 14 Keith Miller 2019-01-30 13:49:23 PST
Created attachment 360613 [details]
Patch for landing
Comment 15 Keith Miller 2019-01-30 13:49:35 PST
Created attachment 360614 [details]
Patch for landing
Comment 16 Mark Lam 2019-01-30 13:53:32 PST
Comment on attachment 360614 [details]
Patch for landing

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

> Source/JavaScriptCore/ChangeLog:9
> +        * assembler/MacroAssembler.h:
> +        (JSC::MacroAssembler::mul32):

Your ChangeLog didn't include assembler/testmasm.cpp.
Comment 17 Mark Lam 2019-01-30 13:54:06 PST
Comment on attachment 360614 [details]
Patch for landing

Cancelling commit for now.
Comment 18 Keith Miller 2019-01-30 14:00:11 PST
Created attachment 360617 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2019-01-30 14:42:17 PST
Comment on attachment 360617 [details]
Patch for landing

Clearing flags on attachment: 360617

Committed r240731: <https://trac.webkit.org/changeset/240731>
Comment 20 WebKit Commit Bot 2019-01-30 14:42:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Radar WebKit Bug Importer 2019-01-30 14:43:31 PST
<rdar://problem/47682256>