Wasmtime GitHub notifications bot (Jun 25 2021 at 15:29): Wasmtime GitHub notifications bot (Jun 25 2021 at 15:29):akirilov-arm opened PR #3035 from simd_i16x8_q15mulr_sat_s to main.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:51):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:51):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:51):cfallin created PR review comment:
The consolidation of all of the
unimplemented!()cases into the wildcard match below, here and in the othermatches (s390x, x64, interpreter), is definitely valid, but IMHO at least, listing the unimplemented opcodes explicitly serves a useful purpose: it ensures that we don't miss an opcode when we go to eventually implement functionality. Tests also ensure this in a way, but that's more indirect (relies on 100% test coverage); and it is also useful sometimes to actually see what is missing.Open to other views on this, though; especially interested in what @abrown @jlb6740 @uweigand think, as other backend-focused folks.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:53):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:53):cfallin created PR review comment:
(And
ConstAddrwhere I placed this line-comment maybe isn't the best example, but e.g. all the SIMD loads were listed out in s390x, whereas an implementer would now likely go through a run-tests, fix-one-panic, retry loop to find all that's needed.)
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:54):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 16:54):uweigand created PR review comment:
Agreed, I'd also prefer unimplemented opcodes to be listed explicitly, for the very reason you give.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:09):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:09):akirilov-arm created PR review comment:
@cfallin I based this approach on the x64 backend, but I don't mind reverting to the previous one.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:11):akirilov-arm edited PR review comment.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:12):akirilov-arm edited PR review comment.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:26):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:26):akirilov-arm created PR review comment:
Also, with Rust there is a quick hack to check what is missing - just comment out the wildcard case and try to rebuild :-).
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:33):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:33):cfallin created PR review comment:
Ah, yes, we are not consistent with the above in
x64; thanks for pointing that out! I just created #3037 (and this was a good exercise as I learned about a few opcodes we hadn't filled in yet).
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:42):akirilov-arm submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:42):akirilov-arm created PR review comment:
Speaking of consistency - notice that both in the AArch64 and the s390x backends some matches use
panic!()for unimplemented operations, while others -unimplemented!(). While it doesn't change program behaviour much, IMHO all should use the latter, so I can clean that up instead - open to opinions (from @uweigand as well).
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:44):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:44):cfallin created PR review comment:
Ah, yes, that would be a welcome cleanup as well; thanks!
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:48):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Jun 25 2021 at 18:48):uweigand created PR review comment:
Agreed!
Wasmtime GitHub notifications bot (Jun 28 2021 at 16:14):akirilov-arm updated PR #3035 from simd_i16x8_q15mulr_sat_s to main.
Wasmtime GitHub notifications bot (Jun 28 2021 at 16:28):cfallin submitted PR review.
Wasmtime GitHub notifications bot (Jun 28 2021 at 16:49):cfallin merged PR #3035.
Last updated: Oct 23 2024 at 20:03 UTC