wasmtime / PR #1789 x86_32: legalize {istore,sload,uload}... · git-wasmtime

This seems to use an I32 pointer (the controlling type) to load the low 32 bits of a register and then sign-extend to 64 bits (from the instruction definition). In 32-bit x86, what is this operation supposed to be doing? If there are no 64-bit registers available for this sign-extension, is sextend.I64 going to legalize further to something else? And is that actually what you want? E.g. if bit 31 is 0, should another register be zeroed out; or if bit 31 is 1, should another register be set to all 1s?

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:28):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:29):

abrown created PR Review Comment:

This uses an I32 pointer (the controlling type) to load the low 32 bits of a register and then just extend (unsigned) to 64 bits. In 32-bit x86, I would think that uextend.I64 should just zero out another register?

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:34):

whitequark submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:34):

whitequark created PR Review Comment:

is sextend.I64 going to legalize further to something else?

Yes. In cases that I see (and I get no new legalizer failures with the *32 instructions) sextend.I64 is then processed by isplit, and the top half is usually just thrown away. I have no special insight for why Cranelift emits sload32 instructions though.

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:35):

whitequark submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 00:35):

whitequark created PR Review Comment:

You're correct that this could be replaced with just iconst, I kept it symmetric but there's no reason other than that.

Wasmtime GitHub notifications bot (Jun 04 2020 at 06:49):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 06:49):

bjorn3 created PR Review Comment:

sload32 instuctions are emitted for a certain WASM instruction.

Wasmtime GitHub notifications bot (Jun 04 2020 at 06:50):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 06:50):

bjorn3 created PR Review Comment:

I believe that is exactly how uextend.i64 is legalized on i686.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:58):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:58):

abrown created PR Review Comment:

Ok, yeah I see it in code_translator.rs. I think a summary of our back and forth here would be helpful as a comment on these legalizations. The only other remaining issue is: why can't this be in the widen group? @bjorn3, is that clear to you?

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:59):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 16:59):

abrown created PR Review Comment:

:+1:

Wasmtime GitHub notifications bot (Jun 04 2020 at 17:01):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 17:01):

bjorn3 created PR Review Comment:

The ctrl typevar is I32, which matches the native pointer size on x86_32, so it requires the expand group. widen is only for ctrl typevars that are smaller than the native pointer size.

Wasmtime GitHub notifications bot (Jun 04 2020 at 17:04):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Jun 04 2020 at 17:04):

abrown created PR Review Comment:

Ah, good. Yeah, let's add that nice detail to the comment as well.

Wasmtime GitHub notifications bot (Jun 25 2020 at 18:49):

alexcrichton edited PR #1789 from x86_32-legalize-loadstore32.i64 to main:

Fixes #1747.

Wasmtime GitHub notifications bot (Jul 05 2020 at 08:50):

whitequark updated PR #1789 from x86_32-legalize-loadstore32.i64 to main:

Fixes #1747.

Wasmtime GitHub notifications bot (Jul 05 2020 at 08:50):

whitequark closed without merge PR #1789.

Last updated: Nov 22 2024 at 17:03 UTC