akirilov-arm opened PR #3035 from simd_i16x8_q15mulr_sat_s
to main
.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
The consolidation of all of the
unimplemented!()
cases into the wildcard match below, here and in the othermatch
es (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.
cfallin submitted PR review.
cfallin created PR review comment:
(And
ConstAddr
where 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.)
uweigand submitted PR review.
uweigand created PR review comment:
Agreed, I'd also prefer unimplemented opcodes to be listed explicitly, for the very reason you give.
akirilov-arm submitted PR review.
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.
akirilov-arm edited PR review comment.
akirilov-arm edited PR review comment.
akirilov-arm submitted PR review.
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 :-).
cfallin submitted PR review.
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).
akirilov-arm submitted PR review.
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).
cfallin submitted PR review.
cfallin created PR review comment:
Ah, yes, that would be a welcome cleanup as well; thanks!
uweigand submitted PR review.
uweigand created PR review comment:
Agreed!
akirilov-arm updated PR #3035 from simd_i16x8_q15mulr_sat_s
to main
.
cfallin submitted PR review.
cfallin merged PR #3035.
Last updated: Dec 23 2024 at 12:05 UTC