Stream: git-wasmtime

Topic: wasmtime / PR #1789 x86_32: legalize {istore,sload,uload}...


view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 17:46):

whitequark edited PR #1789 from x86_32-legalize-loadstore32.i64 to master:

Fixes #1747.

Doesn't work yet due to https://github.com/bytecodealliance/wasmtime/issues/1747#issuecomment-636098907.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:39):

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

Fixes #1747.

Doesn't work yet due to https://github.com/bytecodealliance/wasmtime/issues/1747#issuecomment-636098907.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:46):

whitequark edited PR #1789 from x86_32-legalize-loadstore32.i64 to master:

Fixes #1747.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 19:46):

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

Fixes #1747.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 20:56):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 20:56):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2020 at 20:56):

bjorn3 created PR Review Comment:

    v1 = sload32.i32 v0

same for uload32.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2020 at 05:59):

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

Fixes #1747.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2020 at 05:59):

whitequark has marked PR #1789 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2020 at 05:59):

whitequark created PR Review Comment:

Done. I'm not sure why it worked before the change...

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2020 at 05:59):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:17):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:17):

abrown created PR Review Comment:

In this legalization we are trying to store the lower 32 bits of an I64. It makes sense to me that this would be in narrow.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:19):

abrown edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:25):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:25):

abrown created PR Review Comment:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:28):

abrown submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:34):

whitequark submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 00:35):

whitequark submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 06:49):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 06:49):

bjorn3 created PR Review Comment:

sload32 instuctions are emitted for a certain WASM instruction.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 06:50):

bjorn3 submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 16:58):

abrown submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 16:59):

abrown submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 16:59):

abrown created PR Review Comment:

:+1:

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 17:01):

bjorn3 submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 04 2020 at 17:04):

abrown submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jun 25 2020 at 18:49):

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

Fixes #1747.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2020 at 08:50):

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

Fixes #1747.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 05 2020 at 08:50):

whitequark closed without merge PR #1789.


Last updated: Dec 23 2024 at 13:07 UTC