Bug 165883

Summary: WebAssembly: Add various low hanging fruit that will allow us to run the LLVM torture tests in Wasm
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, commit-queue, fpizlo, ggaren, gskachkov, jfbastien, keith_miller, mark.lam, msaboff, oliver, ticaiolima, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch keith_miller: review+

Saam Barati
Reported 2016-12-14 17:18:03 PST
...
Attachments
patch (19.27 KB, patch)
2016-12-14 17:56 PST, Saam Barati 		keith_miller: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Saam Barati
Comment 1 2016-12-14 17:56:36 PST
Created attachment 297152 [details] patch
WebKit Commit Bot
Comment 2 2016-12-14 17:59:15 PST
Attachment 297152 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:455: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:461: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] ERROR: Source/JavaScriptCore/wasm/WasmModuleParser.cpp:516: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/WasmModuleParser.cpp:523: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:132: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] ERROR: Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:140: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 6 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
JF Bastien
Comment 3 2016-12-14 18:00:38 PST
Comment on attachment 297152 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297152&action=review Looks good otherwise. > JSTests/wasm/Builder.js:196 > + assert.isNumber(index, `Memory exports only support number indices right now`); FIXME + bug? > JSTests/wasm/js-api/table.js:93 > + assertBadBinary(builder, badExportString); I'd really rather use assert.throws, I have to update the other tests to it right now. > JSTests/wasm/js-api/test_memory.js:156 > + return; ?
Saam Barati
Comment 4 2016-12-14 19:10:25 PST
Comment on attachment 297152 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297152&action=review >> JSTests/wasm/js-api/test_memory.js:156 >> + return; > > ? This should be removed.
Keith Miller
Comment 5 2016-12-14 19:14:54 PST
Comment on attachment 297152 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=297152&action=review r=me. > JSTests/wasm/js-api/table.js:279 > + assert.truthy(table === instance.exports.table); > + assert.truthy(table === instance.exports.table2); can't you do assert.eq? >> JSTests/wasm/js-api/test_memory.js:156 >> + return; > > ? Test passes! > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:462 > + Value* partialResult = m_currentBlock->appendNew<Value>(m_proc, BitAnd, Origin(), > + value, > + m_currentBlock->appendNew<Const32Value>(m_proc, Origin(), 0x0000ffff)); Nit, I would do this as: Value* partialResult = m_currentBlock->appendNew<Value>(m_proc, BitAnd, Origin(), value, m_currentBlock->appendNew<Const32Value>(m_proc, Origin(), 0x0000ffff));
Saam Barati
Comment 6 2016-12-14 19:31:42 PST
landed in: https://trac.webkit.org/changeset/209850
Saam Barati
Comment 7 2016-12-14 19:33:01 PST
*** Bug 165684 has been marked as a duplicate of this bug. ***
Saam Barati
Comment 8 2016-12-14 19:33:15 PST
*** Bug 165671 has been marked as a duplicate of this bug. ***
Saam Barati
Comment 9 2016-12-14 19:33:35 PST
*** Bug 165782 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.