mul32 should convert powers of 2 to an lshift
Created attachment 360441 [details] Patch
Comment on attachment 360441 [details] Patch r=me
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 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.
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 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.
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
(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 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.
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
Created attachment 360449 [details] Patch for landing
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.
(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.
Created attachment 360613 [details] Patch for landing
Created attachment 360614 [details] Patch for landing
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 on attachment 360614 [details] Patch for landing Cancelling commit for now.
Created attachment 360617 [details] Patch for landing
Comment on attachment 360617 [details] Patch for landing Clearing flags on attachment: 360617 Committed r240731: <https://trac.webkit.org/changeset/240731>
All reviewed patches have been landed. Closing bug.
<rdar://problem/47682256>