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

subotic · 2024-12-06T16:07:02Z

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 · 2024-12-06T16:08:06Z

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

crates/polkavm-common/src/program.rs

koute · 2024-12-07T08:19:56Z

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

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

subotic · 2024-12-07T10:43:45Z

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

subotic · 2024-12-08T11:53:50Z

@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 · 2024-12-08T13:08:57Z

@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 · 2024-12-13T11:22:42Z

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 · 2024-12-13T13:01:17Z

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

subotic · 2024-12-19T08:22:23Z

@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.

koute

Thanks for the PR!

crates/polkavm-common/src/cast.rs

tools/spectool/src/main.rs

koute · 2025-01-06T06:17:07Z

crates/polkavm-common/src/assembler.rs

+                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));
+                }


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)

@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?

koute · 2025-01-06T06:25:03Z

tools/spectool/src/main.rs

+        let rhs = polkavm_common::utils::parse_imm(rhs)
+            .map(|i| {
+                if i < 0 {
+                    cast(cast(i).to_i64_sign_extend()).to_unsigned()
+                } else {
+                    cast(cast(i).to_unsigned()).to_u64()
+                }
+            })
+            .or_else(|| polkavm_common::utils::parse_imm64(rhs).map(|i| cast(i).to_unsigned()))


This should work similar to how I described load_imm should work (so what value you see is what you get if it's in hex, and if it's like e.g. -4 then it should be immediately parsed as i64)

now using the new parse_immediate function.

koute · 2025-01-06T06:33:10Z

tools/spectool/spec/src/inst_add_32.txt

+pre: a0 = 1
+pre: a1 = 2


For these 32-bit addition tests it would be nice to have:

Both input value are larger than 32-bit, to show that the 32-bit version effectively truncates them

Have two versions of the test - one which results in sign extension (the 31th bit of the result is one) and one which results in no sign extension (the 31th bit of the result is zero), to show the sign extension mechanic

I've added two more programs. Can you please take a look and see if this is what you where thinking?

koute reviewed Dec 7, 2024

View reviewed changes

crates/polkavm-common/src/program.rs Outdated Show resolved Hide resolved

subotic changed the title ~~add 64 bit test programs~~ spectool: add 64-bit test programs Dec 18, 2024

subotic marked this pull request as ready for review December 19, 2024 08:22

koute reviewed Jan 6, 2025

View reviewed changes

koute mentioned this pull request Jan 6, 2025

spectool: Execute JSON test cases #245

Open

subotic changed the title ~~spectool: add 64-bit test programs~~ spectool: add 64-bit test programs (GP-0.5.3) Jan 9, 2025

subotic requested a review from koute January 9, 2025 21:12

subotic added 13 commits January 9, 2025 22:14

spectool: add 64 bit support

1ca7ec8

spectool: add 64 bit post values

6cce772

spectool: fix current tests

7e1710b

spectool: add 64 bit fixes and test cases

dc5a3f7

spectool: fmt

a99147c

spectool: fmt

b96a71e

spectool: add 64 bit test cases

2548938

spectool: add 64 bit test cases

d6291f9

fmt

e77ee04

fix test

1ba0f97

fix tests (probably not correctly)

5e4c67e

incorporate review

a4a36f6

load_imm changes and fixes

b631909

subotic force-pushed the subotic_add_64_bit_test_programs branch from fdd41f4 to b631909 Compare January 9, 2025 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

subotic commented Dec 6, 2024 •

edited

Loading

subotic commented Dec 6, 2024

koute commented Dec 7, 2024

subotic commented Dec 7, 2024

subotic commented Dec 8, 2024 •

edited

Loading

subotic commented Dec 8, 2024 •

edited

Loading

koute commented Dec 13, 2024

subotic commented Dec 13, 2024

subotic commented Dec 19, 2024

koute left a comment

koute Jan 6, 2025

subotic Jan 9, 2025

koute Jan 6, 2025

subotic Jan 9, 2025

koute Jan 6, 2025

subotic Jan 8, 2025

		pre: a0 = 1
		pre: a1 = 2

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

Are you sure you want to change the base?

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

Conversation

subotic commented Dec 6, 2024 • edited Loading

subotic commented Dec 6, 2024

koute commented Dec 7, 2024

subotic commented Dec 7, 2024

subotic commented Dec 8, 2024 • edited Loading

subotic commented Dec 8, 2024 • edited Loading

koute commented Dec 13, 2024

subotic commented Dec 13, 2024

subotic commented Dec 19, 2024

koute left a comment

Choose a reason for hiding this comment

koute Jan 6, 2025

Choose a reason for hiding this comment

subotic Jan 9, 2025

Choose a reason for hiding this comment

koute Jan 6, 2025

Choose a reason for hiding this comment

subotic Jan 9, 2025

Choose a reason for hiding this comment

koute Jan 6, 2025

Choose a reason for hiding this comment

subotic Jan 8, 2025

Choose a reason for hiding this comment

subotic commented Dec 6, 2024 •

edited

Loading

subotic commented Dec 8, 2024 •

edited

Loading

subotic commented Dec 8, 2024 •

edited

Loading