[Table64Lowering] Don't assume that all segments are from 64-bit tables

[sbc100]

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.

[tlively]

Why does it matter what order things are visited in? Is it not possible to do the lowering independently for segments and memories/tables?

[sbc100]

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 is64() will be false.

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.

[kripken]

lgtm % comments

[tlively]

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?

[sbc100]

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 memory->is64() as the guard. So there is already an ordering assumption here. It would seem a little odd to me to use a different check for segments.. that would probably require an comment explaining the ordering differences.

[sbc100]

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?

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).

[tlively]

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.

[kripken]

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?

[kripken]

(though they aren't actually nested like expressions are, but still, there is a logical order)

[sbc100]

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.

There are already assumptions about ordering in these 2 passes. All of the other visit functions assume they run before the table or memory is visited because they all depend on being able to check is64. Are those existing assumption OK, or should we remove all the assumption and perform the table/memory mods after the pass is done?

[tlively]

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.

[sbc100]

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.