Bug 193957 – mul32 should convert powers of 2 to an lshift

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Keith Miller]

mul32 should convert powers of 2 to an lshift

[Keith Miller]

Created attachment 360441 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 360441 [details]
Patch

r=me

[Mark Lam]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

(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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Keith Miller]

Created attachment 360449 [details]
Patch for landing

[Mark Lam]

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.

[Mark Lam]

(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.

[Keith Miller]

Created attachment 360613 [details]
Patch for landing

[Keith Miller]

Created attachment 360614 [details]
Patch for landing

[Mark Lam]

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.

[Mark Lam]

Comment on attachment 360614 [details]
Patch for landing

Cancelling commit for now.

[Keith Miller]

Created attachment 360617 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 360617 [details]
Patch for landing

Clearing flags on attachment: 360617

Committed r240731: <https://trac.webkit.org/changeset/240731>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47682256>