Stream: git-wasmtime

Topic: wasmtime / issue #4514 x64: Shrink Inst from 72 to 48 bytes


view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 21:46):

github-actions[bot] commented on issue #4514:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 22:02):

jameysharp commented on issue #4514:

I can't find where targets_for_term is actually used. It seems strange to me that the combination of default_target plus targets is always identical to targets_for_term. I would think you could save 24 bytes by just removing that field.

That said... I'd be tempted instead to remove default_target and targets, which I think should save 28 bytes, at the cost of needing an indirection to dig default_target out of the heap allocation instead of having it right there in the Inst. But when default_target is used, so is the rest of the targets array, so I think that shouldn't impact performance.

I also don't see that these vectors are modified after they're constructed. Box<[MachLabel]> is 16 bytes versus 24 bytes for Vec<MachLabel>, so I think you can shave a total of 36 bytes off by doing the above and then switching the targets array to a boxed slice.

This could be a terrible plan for reasons I don't know, obviously. :grin:

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 22:06):

cfallin commented on issue #4514:

They originally existed to satisfy queries from regalloc.rs; although, thank you @jameysharp for persisting here, it seems indeed that targets_for_term is entirely dead now, and I believe targets as well. (MachTerminator used to carry a slice which was built from one of these and regalloc.rs used to query successors directly from branch Insts built from the other; I don't remember why they were separate but there was a reason...).

So perhaps they can just be removed entirely!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 22:13):

jameysharp commented on issue #4514:

targets is used along with default_target in inst/emit.rs to emit the target addresses, so I think we still need one copy, just not two; and I'd still advocate for using the single boxed slice for all targets. Unless there's someplace else to get the targets from that's still around at that point? But I assume cutting the size of this case in half already makes it not the largest variant in this enum any more.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 22:15):

elliottt commented on issue #4514:

I'm happy to try out the single boxed slice approach, thanks for chasing the uses of these vectors down @jameysharp!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 23:56):

elliottt commented on issue #4514:

Removing the targets_for_tree field got some additional memory back:

104372
104192
104312
104372
104404
104304
104412
104368
104904
104400

Experimenting with a boxed slice and an non-boxed SmallVec both yielded more memory consumed pretty consistently, so for now a boxed SmallVec seems like the winner. I'm happy to experiment a bit more with this, but that'll need to wait until next week.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2022 at 00:32):

elliottt commented on issue #4514:

@cfallin, I made a few more changes since you reviewed: the targets_for_term field is gone, and I moved some aliases around a bit that were re-defined in other backends.


Last updated: Dec 23 2024 at 12:05 UTC