Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace more execution; fix array.new/init_data rules #88

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

rossberg
Copy link
Collaborator

@jaehyun1ee, @f52985, as discussed today, this fixes a bug in the reduction of array.new_data/init_data. The new version requires the inverse of the packconst helper, which I'm not sure how to implement, hence the failure below.

 ===== ../../test-interpreter/spec-test-3/gc/array_init_data.wast =====
-- 31/31 (100.00%)
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:95.1-95.85 (Failure("Invalid assignment on value 0x63: CallE (packconst, [ VarE (zt), VarE (c) ]) @8-reduction.watsup:522.21-522.38"))
+Result: 0 : [i32]
+Expect: 99 : [i32]
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:97.1-97.70 (Error(_, "wrong return values"))
+Result: 0 : [i32]
+Expect: 100 : [i32]
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:98.1-98.71 (Error(_, "wrong return values"))
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:101.1-101.89 (Failure("Invalid assignment on value 0x6766: CallE (packconst, [ VarE (zt), VarE (c) ]) @8-reduction.watsup:522.21-522.38"))
+Result: 0 : [i32]
+Expect: 26_470 : [i32]
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:103.1-103.78 (Error(_, "wrong return values"))
+Result: 0 : [i32]
+Expect: 26_984 : [i32]
+- Test failed at ../../test-interpreter/spec-test-3/gc/array_init_data.wast:104.1-104.78 (Error(_, "wrong return values"))
+- 25/31 (80.65%)

@rossberg rossberg merged commit 414f1ad into main Mar 20, 2024
3 of 5 checks passed
@rossberg rossberg deleted the exec branch March 20, 2024 17:25
@rossberg
Copy link
Collaborator Author

rossberg commented Mar 21, 2024

Sleeping over this I realised that my fix wasn't quite what we wanted, because packconst is, in fact, not invertible: because it wraps its argument, it is a lossy operation. So I changed it the other way round, using unpackconst on the RHS, which also fixed the test failures.

(For the semantics of these ops it wouldn't matter which inverse value to pick, because they all result in the same value being written in the next step. So underspecifying it as before wasn't incorrect per se, but it still resulted in a local non-determinism that we'd like to avoid.)

In short, problem solved, you can ignore the above. I hope you haven't yet wasted time on it...

@f52985
Copy link
Collaborator

f52985 commented Mar 21, 2024

That's great! I was just about to have a look at it, so no time wasted at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants