Stream: git-wasmtime

Topic: wasmtime / PR #9279 Align the start of stackslots instead...

Wasmtime GitHub notifications bot (Sep 19 2024 at 10:54):

tertsdiepraam opened PR #9279 from tertsdiepraam:fix-stackslot-alignment to bytecodealliance:main:

Hi! I think that the stackslots weren't properly aligned, because the padding for the alignment is being added to the end of the slot instead of at the start, meaning that the slot itself isn't really being aligned.

This was creating issues for me where I requested a slot that was aligned at 16 bytes, but in reality it wasn't.

With this change this case from the tests:

ss0 = explicit_slot 8, align=16
ss1 = explicit_slot 8, align=16
ss2 = explicit_slot 4
ss3 = explicit_slot 4

gets these offsets:

ss0: 0
ss1: 16 (because size of ss0 + padding to get to 16)
ss2: 24 (because size of ss1)
ss3: 32 (because size of ss2 + padding to get to the word size of 8)

Note that the align=16 of ss0 is essentially a noop now.

And this case from a test I added:

ss0 = explicit_slot 8
ss1 = explicit_slot 32, align=16

gets these offsets:

ss0: 0
ss1: 16 (because size of ss0 + padding to get to 16)

I hope I didn't make any mistakes here or misinterpreted what stackslot alignment should do. Let me know if I need to change anything!

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 19 2024 at 10:54):

tertsdiepraam requested cfallin for a review on PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 10:54):

tertsdiepraam requested wasmtime-compiler-reviewers for a review on PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 10:56):

tertsdiepraam edited PR #9279:

Hi! I think that the stackslots weren't properly aligned, because the padding for the alignment is being added to the end of the slot instead of at the start, meaning that the slot itself isn't really being aligned.

This was creating issues for me where I requested a slot that was aligned at 16 bytes, but in reality it wasn't.

With this change this case from the tests:

ss0 = explicit_slot 8, align=16
ss1 = explicit_slot 8, align=16
ss2 = explicit_slot 4
ss3 = explicit_slot 4

gets these offsets:

ss0: 0
ss1: 16 (because size of ss0 + padding to get to 16)
ss2: 24 (because size of ss1)
ss3: 32 (because size of ss2 + padding to get to the word size of 8)

Note that the align=16 of ss0 is essentially a noop now.

And this case from a test I added:

ss0 = explicit_slot 8
ss1 = explicit_slot 32, align=16

gets these offsets:

ss0: 0
ss1: 16 (because size of ss0 + padding to get to 16)

I hope I didn't make any mistakes here or misinterpreted what stackslot alignment should do. Let me know if I need to change anything!

The issue was originally found here: https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/Retrieve.20alignment.20of.20architecture/near/471006324

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:01):

tertsdiepraam updated PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:22):

bal-e submitted PR review.

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:22):

bal-e created PR review comment:

Should this cause a panic?

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:24):

tertsdiepraam submitted PR review.

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:24):

tertsdiepraam created PR review comment:

Oops committed by accident thanks

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:26):

tertsdiepraam updated PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:27):

tertsdiepraam updated PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 11:31):

tertsdiepraam updated PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 15:42):

cfallin submitted PR review:

Thanks a bunch for fixing this!

Wasmtime GitHub notifications bot (Sep 19 2024 at 16:12):

cfallin merged PR #9279.

Wasmtime GitHub notifications bot (Sep 19 2024 at 16:17):

tertsdiepraam commented on PR #9279:

Thanks!

Last updated: Dec 23 2024 at 12:05 UTC