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

spectool: add 64-bit test programs (GP-0.5.3) #238

Merged
merged 14 commits into from
Jan 20, 2025

Conversation

subotic
Copy link
Collaborator

@subotic subotic commented Dec 6, 2024

  • added test programs for 64-bit instructions as per GP-0.5.3
  • extended "polkavm-common/assembler" to support 64-bit instructions
  • fixed some unit tests. Need pointers on how to fix them correctly.

@subotic
Copy link
Collaborator Author

subotic commented Dec 6, 2024

@koute Could you maybe take a look if I'm on the right track?

@koute
Copy link
Collaborator

koute commented Dec 7, 2024

@koute Could you maybe take a look if I'm on the right track?

Yes, you're on the right track. (:

@subotic
Copy link
Collaborator Author

subotic commented Dec 7, 2024

@koute Also, is it possible to authorise me to run the workflows? I don't want to bother you all the time.

@subotic
Copy link
Collaborator Author

subotic commented Dec 8, 2024

@koute Is it correct, that load_imm given an 4 byte immediate like 0xdeadbeef, the post condition of the register should be 0x00000000deadbeef and not 0xffffffffdeadbeef? Or should the logic be that it is always sign extended?

@subotic
Copy link
Collaborator Author

subotic commented Dec 8, 2024

@koute how to discern between load_imm and load_imm64 if the disassembly for both is the same {d} = 0x{a:x}? Can/should I add to load_imm64 something like i64 a0 = 10? Or am I misunderstanding load_imm64 and the value of the immediate needs to be always larger then i32?

@koute
Copy link
Collaborator

koute commented Dec 13, 2024

Also, is it possible to authorise me to run the workflows? I don't want to bother you all the time.

Not entirely sure how to do that; I've added you as a read-only contributor - hopefully that will make the tests auto run.

Is it correct, that load_imm given an 4 byte immediate like 0xdeadbeef, the post condition of the register should be 0x00000000deadbeef and not 0xffffffffdeadbeef? Or should the logic be that it is always sign extended?

If it's a negative number (i.e. the most significant bit is set) then yes, it should be sign extended (but the VM takes care of that automatically).

how to discern between load_imm and load_imm64 if the disassembly for both is the same {d} = 0x{a:x}? Can/should I add to load_imm64 something like i64 a0 = 10? Or am I misunderstanding load_imm64 and the value of the immediate needs to be always larger then i32?

If a value can be encoded with a load_imm then load_imm should always be used. If the value cannot be encoded with load_imm then only then load_imm_64 should be emitted.

(And we could also have an override to force load_imm_64 for testing, e.g. maybe {reg} = i64 {value}?)

@subotic
Copy link
Collaborator Author

subotic commented Dec 13, 2024

Great pointers thank you very much. This clears and answers a lot.

@subotic subotic changed the title add 64 bit test programs spectool: add 64-bit test programs Dec 18, 2024
@subotic subotic marked this pull request as ready for review December 19, 2024 08:22
@subotic
Copy link
Collaborator Author

subotic commented Dec 19, 2024

@koute could you maybe take a look again? I had to change some unit tests, which should probably be done differently, but for this I would need your input.

Copy link
Collaborator

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

crates/polkavm-common/src/cast.rs Outdated Show resolved Hide resolved
tools/spectool/src/main.rs Outdated Show resolved Hide resolved
Comment on lines 446 to 463
if let Some(index) = rhs.find("i64 ") {
let rhs = rhs[index + 4..].trim();
if let Some(imm) = parse_imm(rhs) {
let imm = cast(imm).to_i64_sign_extend();
emit_and_continue!(Instruction::load_imm64(dst.into(), imm as u64));
}
if let Some(imm64) = parse_imm64(rhs) {
emit_and_continue!(Instruction::load_imm64(dst.into(), imm64 as u64));
}
}

if let Some(imm) = parse_imm(rhs) {
emit_and_continue!(Instruction::load_imm(dst.into(), imm as u32));
}

if let Some(imm64) = parse_imm64(rhs) {
emit_and_continue!(Instruction::load_imm64(dst.into(), imm64 as u64));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is partially my fault because of how the load_imm instructions are currently printed out (I need to change it, or you could change it if you want), but here's how it should work:

  • a0 = $value in the assembly/disassembly code should always result it exactly the $value being loaded into the register (what you see is what you get)
  • by default if the immediate can be represented with load_imm then it always should use load_imm, and only should fall back to load_imm64 when it cannot
  • i64 prefix should always force load_imm64
  • load_imm can be used to load immediates between 0 and 0x7fffffff (inclusive), and also any immediates where all upper 33 bits are set (because it sign extends)

So, for example:

  • a0 = 0xffffffff should result in 0xffffffff being assigned, so this must use load_imm64 (because load_imm will sign extend the value from 32-bits to 64-bits)
  • a0 = 0xffffffff87654321 should result in 0xffffffff87654321 being assigned and should use load_imm by default (because all of the upper bits are 1s so load_imm with argument 0x87654321 can be used)
  • a0 = -2 should result in 0xfffffffffffffffe being assigned and should use load_imm by default (because all of the upper bits are 1s so load_imm with argument 0xfffffffe can be used)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@koute Ok, I think I got it now. Can you take a look at the new parse_immediate function and see if it correctly captures the described behaviour?

tools/spectool/src/main.rs Outdated Show resolved Hide resolved
tools/spectool/spec/src/inst_add_32.txt Show resolved Hide resolved
@subotic subotic changed the title spectool: add 64-bit test programs spectool: add 64-bit test programs (GP-0.5.3) Jan 9, 2025
@subotic subotic requested a review from koute January 9, 2025 21:12
@subotic subotic force-pushed the subotic_add_64_bit_test_programs branch from fdd41f4 to b631909 Compare January 9, 2025 21:14
@subotic subotic force-pushed the subotic_add_64_bit_test_programs branch 2 times, most recently from 96eca0c to 02de4ef Compare January 17, 2025 15:33
fix(spectool): parsing immediates

lint

fix test
@subotic subotic force-pushed the subotic_add_64_bit_test_programs branch from 182f559 to 455667b Compare January 17, 2025 15:47
@subotic subotic requested a review from koute January 17, 2025 16:00
@koute koute merged commit cdc0b70 into paritytech:master Jan 20, 2025
11 checks passed
@subotic subotic deleted the subotic_add_64_bit_test_programs branch January 20, 2025 18:24
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