-
Notifications
You must be signed in to change notification settings - Fork 710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Table64Lowering] Don't assume that all segments are from 64-bit tables #6599
Conversation
This allows modules to contains both 32-bit and 64-bit segment. In order to check the table/memory state when visiting segments we need to ensure that memories/tables are visited only after their segments. The comments in visitTable/visitMemory already assumed this but its wasn't actually true in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it matter what order things are visited in? Is it not possible to do the lowering independently for segments and memories/tables?
Because when we visit segments (in fact when we visit everything except the memoy/table) we need to check the associated table/memory to see if its 64-bit. If we visit the table/memory first then we will have already converted to 32-bit so The alternative to relying on this ordering would be to avoid mutating the memories/tables when we visit them and queue up the lowering for the end of the pass.. but that seems like more work than this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments
Sorry, I still don't see why this is necessary. When we lower segments, can't we just check whether their offset type is i64 instead of depending on the table type? |
We could do that I suppose yes. But everywhere else in these files we use |
In addition, is seems a little more direct to me to check that property of the table that we are interested in rather than the segment offset (which happens to match the table type for valid modules due to the validation rules). |
I would much rather have separate checking logic for segments than take a dependency on the visit order. That seems like a bad abstraction break. Imagine we had multiple passes that wanted different visit orders so they could be slightly simpler. There would be no way to give them all the precise order they wanted. This pass should not be special, especially since there's such a straightforward way to remove the dependency on visit order that aligns with the spec. |
I was a bit worried about that too, but then I started to look at it as part of postorder. That is, we visit expressions before the Function they are in, so it seems ok to assume we visit segments before their memory/table? |
(though they aren't actually nested like expressions are, but still, there is a logical order) |
There are already assumptions about ordering in these 2 passes. All of the other |
I would prefer to remove all the assumptions about visitation order and have everything based on local information. That will also let the lowering be parallelizable. |
Ok, I did that in #6600. Was easier than I thought it would be. |
This allows modules to contains both 32-bit and 64-bit segment.
In order to check the table/memory state when visiting segments we need to ensure that memories/tables are visited only after their segments.
The comments in visitTable/visitMemory already assumed this but its wasn't actually true in practice.