Stream: git-wasmtime

Topic: wasmtime / issue #3102 cranelift: Fix trampoline args for...

Wasmtime GitHub notifications bot (Jul 21 2021 at 11:35):

bjorn3 commented on issue #3102:

b* are written as rust bool which is 1 byte big. For eg b16 2 bytes would need to be written to ensure that the bool is actually valid. 0xff00 is not a valid bool afaik. In addition this would only work somewhat fine on little-endian systems. On big endian systems, the msb and not the lsb of the bool would be written.

Wasmtime GitHub notifications bot (Jul 21 2021 at 11:47):

afonso360 commented on issue #3102:

So, I'm guessing the solution here would be to write these as u128's in write_value_to:

https://github.com/bytecodealliance/wasmtime/blob/065190f975e7e94650d85d35d3fbea5448a746bd/cranelift/codegen/src/data_value.rs#L68

To ensure that we always clear all bytes, but I'm not sure how this would work on big endian systems.

0xff00 is not a valid bool afaik

Aren't larger boolean sizes meant to work as bitmasks? That is the general impression that I got so far, but I haven't seen anything concrete about this.

Wasmtime GitHub notifications bot (Jul 21 2021 at 11:51):

afonso360 edited a comment on issue #3102:

So, I'm guessing the solution here would be to write these as u128's in write_value_to:

https://github.com/bytecodealliance/wasmtime/blob/065190f975e7e94650d85d35d3fbea5448a746bd/cranelift/codegen/src/data_value.rs#L68

To ensure that we always clear all bytes, but I'm not sure how this would work on big endian systems.
We always reserve a u128 for each slot, so this shouldn't write values out of bounds.

0xff00 is not a valid bool afaik

Aren't larger boolean sizes meant to work as bitmasks? That is the general impression that I got so far, but I haven't seen anything concrete about this.

Wasmtime GitHub notifications bot (Jul 21 2021 at 12:05):

bjorn3 commented on issue #3102:

A b16 would be either stored as 0x0000/0xffff or 0x0000/0x0001. I am not sure which one is the right one. 0xff00 is definitively not right.

Wasmtime GitHub notifications bot (Jul 21 2021 at 13:20):

afonso360 commented on issue #3102:

I updated this to write out the full u128 slot as 1 or 0, but lets wait on some feedback about writing it as all ones instead

Wasmtime GitHub notifications bot (Jul 22 2021 at 17:56):

abrown commented on issue #3102:

Here's context from the docs:

The b1 type represents an abstract boolean value. It can only exist as an SSA value, and can't be directly stored in memory. It can, however, be converted into an integer with value 0 or 1 by the bint instruction (and converted back with icmp_imm with 0).
Several larger boolean types are also defined, primarily to be used as SIMD element types. They can be stored in memory, and are represented as either all zero bits or all one bits.

I've had some conversations about this with @sunfishcode and @cfallin in the past--I'll let them comment here. FWIW, I think the approach in ecb72cc of storing a 0 or a 1 to represent a boolean is fine.

Wasmtime GitHub notifications bot (Aug 06 2021 at 21:31):

cfallin commented on issue #3102:

Just clearing some backlog and seeing this now -- sorry for the delay! A few points:

• Wider bool types are indeed stored as all-ones or all-zeroes, as they're meant to serve as bitmasks.
• For b1, as @abrown notes above, they're not supposed to be stored/loaded from within CLIF, semantically. On all three of x64, aarch64, s390x, we AND out the LSB to implement bint, following the usual "upper bits are undefined" invariant.
• But because we are writing runtime-internal code, we can reason about the combination of our runtime-side stores and generated-code loads; we aren't restricted by "don't load/store b1".

If we are going to do the fully generic thing for bools of all widths, the proper approach I think is to write all-ones (-1i128 as u128) to the u128 slot, then a load of any width will pick up all-ones. So I think this patch is almost there, except for the constant value.

Last updated: Nov 22 2024 at 16:03 UTC