Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Align the start of stackslots instead of the end #9279

Merged

Conversation

tertsdiepraam
Copy link
Contributor

@tertsdiepraam tertsdiepraam commented Sep 19, 2024

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

@tertsdiepraam tertsdiepraam requested a review from a team as a code owner September 19, 2024 10:54
@tertsdiepraam tertsdiepraam requested review from cfallin and removed request for a team September 19, 2024 10:54
@tertsdiepraam tertsdiepraam force-pushed the fix-stackslot-alignment branch from 19c0e50 to 5b43c59 Compare September 19, 2024 11:01
@tertsdiepraam tertsdiepraam force-pushed the fix-stackslot-alignment branch 2 times, most recently from 5d7b1dc to 6c6bad9 Compare September 19, 2024 11:27
The alignment for stackslots was commputed wrongly, because the
end of the slot was being aligned instead of the beginning.
This effectively meant that the _next_ stackslot was being aligned
on the alignment of the current slot.
@tertsdiepraam tertsdiepraam force-pushed the fix-stackslot-alignment branch from 6c6bad9 to 22dbe57 Compare September 19, 2024 11:31
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Sep 19, 2024
Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a bunch for fixing this!

@cfallin cfallin added this pull request to the merge queue Sep 19, 2024
Merged via the queue into bytecodealliance:main with commit d0cdac7 Sep 19, 2024
39 checks passed
@tertsdiepraam
Copy link
Contributor Author

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants