Stream: git-wasmtime

Topic: wasmtime / Issue #1699 Reduce arm64 Inst enum size

Wasmtime GitHub notifications bot (May 14 2020 at 14:39):

github-actions[bot] commented on Issue #1699:

Subscribe to Label Action

cc @bnjbvr

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

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

• bnjbvr: cranelift

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 (May 15 2020 at 05:09):

julian-seward1 commented on Issue #1699:

A nice improvement.

Do you have any insight into why it's still 48 bytes? Given that an arm64 insn can mention at most 4 regs, and that each Reg is 4 bytes, that means that we need 16 bytes to represent the registers, which leaves 32 bytes left over for all the opcode-y info and auxiliary info. Which still feels to me like a lot. So I was wondering about the feasibility of reducing the struct size further, to 32 bytes.

Wasmtime GitHub notifications bot (May 15 2020 at 10:06):

jgouly commented on Issue #1699:

    JTSequence {
        targets: Box<[BranchTarget]>,
        targets_for_term: Box<[BlockIndex]>, // needed for MachTerminator.
        ridx: Reg,
        rtmp1: Writable<Reg>,
        rtmp2: Writable<Reg>,
    },

This is 48 bytes. (3 * 4 bytes for Regs + 2 * 16 for Box<[T]> + padding).
We could make this:

    JTSequence {
        targets_info: Box<NewTargetInfoStruct>,
        ridx: Reg,
        rtmp1: Writable<Reg>,
        rtmp2: Writable<Reg>,
    },

Which should make it 3 * 4 + 8 = 24 bytes. However NewTargetInfoStruct would have 2 Box<[T]> inside of it, so this would be introducing another allocation (could be fine, worth pointing out). It seems there is an unsafe way in we could could allocate space for NewTargetInfoStruct with trailing space for the targets and targets_for_term, but I think that might be overkill.

Also 48 bytes:

    CondBr {
        taken: BranchTarget,
        not_taken: BranchTarget,
        kind: CondBrKind,
    },

Surprising. Looks so simple! BranchTarget is 16 bytes though.

enum BranchTarget {
    Block(BlockIndex),
    ResolvedOffset(isize),
}

I don't think we need the full isize range there (@cfallin ?), so we could encode this ourselves, using the top bit of the isize as the discriminant. Then BranchTarget could shrink to 8 bytes (the size of a single isize)

Other than that, LoadExtName is 39 bytes. This is because it has a field of ExternalName (20 bytes), using Box<ExternalName> would drop the size to 24 bytes.

Otherwise everything is already 32-bytes.

Wasmtime GitHub notifications bot (May 15 2020 at 11:46):

bnjbvr commented on Issue #1699:

However NewTargetInfoStruct would have 2 Box<[T]>

Could NewTargetInfoStrust just contain the two fields without the boxes, avoiding the two supplementary Box allocations?

I don't think we need the full isize range there (@cfallin ?), so we could encode this ourselves, using the top bit of the isize as the discriminant.

Yep, we discussed this and Chris said a byte would be sufficient for this.

Other than that, LoadExtName is 39 bytes. This is because it has a field of ExternalName (20 bytes), using Box<ExternalName> would drop the size to 24 bytes.

Uh, it's silly: ExternalName is 20 bytes because it has variant apparently only used in test cases that contains 17 bytes rounded with padding to 20. If we made this really test-only (there's already a Cargo feature for this purpose in Cranelift), or reduce the TEST_NAME length by one unit, then we could make it only 16 bytes. Then putting it in a box would make sense too.

Wasmtime GitHub notifications bot (May 15 2020 at 12:08):

jgouly commented on Issue #1699:

However NewTargetInfoStruct would have 2 Box<[T]>

Could NewTargetInfoStrust just contain the two fields without the boxes, avoiding the two supplementary Box allocations?

The Boxes it contains are Box<[T]>, which is produced from a Vec<T>. Not single values.

Wasmtime GitHub notifications bot (May 15 2020 at 15:59):

bjorn3 commented on Issue #1699:

Converting a Vec<T> to a Box<[T]> may reallocate if the capacity is higher than the length.

Wasmtime GitHub notifications bot (May 15 2020 at 16:28):

jgouly commented on Issue #1699:

Converting a Vec<T> to a Box<[T]> may reallocate if the capacity is higher than the length.

I don't think that should happen in either of these two uses here, right?: https://github.com/bytecodealliance/wasmtime/commit/f418b7a7006842437e75a584e353cdd6fc59d93d#diff-7d584cb8ba0adceef3f4e7c5eff08c05R2125

Last updated: Nov 22 2024 at 16:03 UTC