Stream: git-wasmtime

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

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:

• cfallin: isle
• fitzgen: isle

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

Learn more.
</details>

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:

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!

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.

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!

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.

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: Nov 22 2024 at 16:03 UTC