Skip to content

Conversation

@koute
Copy link
Collaborator

@koute koute commented Feb 5, 2025

As reported in: w3f/jamtestvectors#3 (comment)

For division instructions we need to round towards zero instead of flooring (the fractional part of the result should always be discarded).

@emielsebastiaan
Copy link
Contributor

emielsebastiaan commented Feb 5, 2025

This is likely an incomplete solution. We implemented these changes and now test vectors for all shar* (Shift Arithmetic Right) testvectors fail. I.e., instructions: 140, 146, 153, 157, 199 & 209
Interestingly enough before this PR our implementation for these instructions were passing.

Example:

  • shar_r_64 (209)
  • Testvector: shift_arithmic_right_64.json

Input

ω_A (register: 7) = 9223372036854775925
ω_B (register: 8) = 3
ω_D (register: 9) = Destination register for outcome

Calculations

Z_8(ω_A) = -9223372036854775691
2**(ω_B % 64) = 8
Division: -9223372036854775691 / 8 = -1152921504606846961,375
Floor/Ceil: rtz(-9223372036854775691 / 8) = rtz(-1152921504606846961,375) = -1152921504606846961
Calculated output: ω_D (register: 9) = Z_inv_8(-1152921504606846961) = 17293822569102704655

Expected Output Test Vectors

ω_D= 17293822569102704654 (!= Calculated output: 17293822569102704655)

Suggested follow-up

  • Either GP definition issue with current shar* instructions: 140, 146, 153, 157, 199 & 209.
  • Or test vectors are incorrect.

@koute
Copy link
Collaborator Author

koute commented Feb 5, 2025

Hm, at least with arithmetic shifts you're correct. I'll double check this. Thanks.

@koute
Copy link
Collaborator Author

koute commented Feb 5, 2025

Okay, I double checked, and I indeed had a brain fart here. Apologies. Only divisions should have this; thanks @emielsebastiaan.

@emielsebastiaan
Copy link
Contributor

Yes confirmed... in all those other modifications you made ealier (140, 146, 153, 157, 199, 209, 213 & 215) the division only protected against overflow. All good now! thanks

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.

3 participants