Stream: git-wasmtime

Topic: wasmtime / issue #5398 Remove unsigned integers in DataValue

Wasmtime GitHub notifications bot (Dec 08 2022 at 16:49):

afonso360 opened issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see for example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly, for example. If a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Wasmtime GitHub notifications bot (Dec 08 2022 at 16:53):

afonso360 edited issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see as an example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly. For example if a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Wasmtime GitHub notifications bot (Dec 08 2022 at 16:54):

afonso360 edited issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see as an example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly. For example if a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Overall this is quite confusing in the interpreter and causes a bunch of casting between types that could probably be avoided.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Wasmtime GitHub notifications bot (Dec 08 2022 at 18:44):

jameysharp labeled issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see as an example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly. For example if a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Overall this is quite confusing in the interpreter and causes a bunch of casting between types that could probably be avoided.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Wasmtime GitHub notifications bot (Dec 08 2022 at 18:44):

jameysharp labeled issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see as an example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly. For example if a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Overall this is quite confusing in the interpreter and causes a bunch of casting between types that could probably be avoided.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Wasmtime GitHub notifications bot (Dec 11 2022 at 01:17):

bahildebrand commented on issue #5398:

Is this still available? If so I'd like to take a crack at it.

Wasmtime GitHub notifications bot (Dec 11 2022 at 10:51):

afonso360 commented on issue #5398:

As far as I'm aware no one has started working on it yet!

Wasmtime GitHub notifications bot (Dec 12 2022 at 08:33):

tnachen commented on issue #5398:

@bahildebrand I'm looking to do this but if you already started let me know, can defer this to you

Wasmtime GitHub notifications bot (Dec 13 2022 at 13:18):

bahildebrand commented on issue #5398:

@tnachen I have already started, but it seems like you might have as well.

Wasmtime GitHub notifications bot (Apr 18 2023 at 14:58):

afonso360 closed issue #5398:

:wave: Hey,

Feature

We should remove the unsigned variants of the DataValue structure.

Benefit

This brings this structure closer to CLIF's idea of a DataValue, which does not distinguish between signed and unsigned in types. Instead in CLIF we only have that distinction in the operations that are performed (see as an example sdiv vs udiv)

It can also bring some issues to the interpreter if not handled correctly. For example if a operation returns a unsigned DataValue it can cause a test failure such as Failed test: run: %usubof_i64(0, 0) == 0, actual: 0 since we parse the values in test cases as signed integers.

Overall this is quite confusing in the interpreter and causes a bunch of casting between types that could probably be avoided.

Implementation

As far as I'm aware the only user of these values is the interpreter. Which casts values to unsigned when it needs to perform operations as unsigned, and then has to cast them back to signed values before returning them.

We can instead separate those operations like we do in CLIF instructions. As an example right now we have Value::div which does signed or unsigned division based on the signedness of the input DataValue, but we can change that to have Value::sdiv and Value::udiv like we have for CLIF instructions.

Alternatives

We can keep using this system and changing the PartialEq implementation to fix some of the issues above, but that does not sound like the best solution to me.

See the following zulip thread for some additional context.

Last updated: Nov 22 2024 at 16:03 UTC