wasmtime / PR #2328 WIP: initial emission of constants · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2328 WIP: initial emission of constants

Wasmtime GitHub notifications bot (Oct 27 2020 at 18:57):

abrown opened PR #2328 from constants to main:

@cfallin, based on what we discussed in Zulip and #1385, here's a first pass on a way to emit constants using MachBuffer::defer_constant. Note that I had to do two mappings--one from Constant to UsedConstant and another from UsedConstant to MachLabel--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improve gen_constant.

Wasmtime GitHub notifications bot (Oct 27 2020 at 18:57):

abrown requested cfallin for a review on PR #2328.

Wasmtime GitHub notifications bot (Oct 27 2020 at 22:44):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin created PR Review Comment:

This is actually inside the per-block loop body, so this would emit one copy of each (function-wide) constant per basic block. I don't think this is what was intended :-)

Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin created PR Review Comment:

Would it be sufficient in all cases to align to the size (so e.g. 8-byte constants are 8-byte-aligned, ...)? Or are there cases where we need more than that?

Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin created PR Review Comment:

I'm trying to think of a better name for this -- would just VCodeConstant be more fitting, do you think, since they live in the VCode?

Wasmtime GitHub notifications bot (Oct 30 2020 at 19:11):

abrown updated PR #2328 from constants to main:

@cfallin, based on what we discussed in Zulip and #1385, here's a first pass on a way to emit constants using MachBuffer::defer_constant. Note that I had to do two mappings--one from Constant to UsedConstant and another from UsedConstant to MachLabel--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improve gen_constant.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:17):

abrown updated PR #2328 from constants to main:

@cfallin, based on what we discussed in Zulip and #1385, here's a first pass on a way to emit constants using MachBuffer::defer_constant. Note that I had to do two mappings--one from Constant to UsedConstant and another from UsedConstant to MachLabel--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improve gen_constant.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:21):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:21):

abrown created PR Review Comment:

Check out the new VCodeConstantData::alignment and let me know what you think.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:37):

abrown updated PR #2328 from constants to main:

@cfallin, based on what we discussed in Zulip and #1385, here's a first pass on a way to emit constants using MachBuffer::defer_constant. Note that I had to do two mappings--one from Constant to UsedConstant and another from UsedConstant to MachLabel--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improve gen_constant.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:38):

abrown updated PR #2328 from constants to main:

@cfallin, based on what we discussed in Zulip and #1385, here's a first pass on a way to emit constants using MachBuffer::defer_constant. Note that I had to do two mappings--one from Constant to UsedConstant and another from UsedConstant to MachLabel--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improve gen_constant.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown created PR Review Comment:

We could make this more granular but I am guessing this is good enough for now?

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown created PR Review Comment:

@cfallin, take a look at this. The compile-time memory usage will definitely be higher if we de-duplicate constants. As you know, there's quite the trade-off here:

either we pay for these structures in compile-time memory

we pay for de-duplication with compile-time CPU usage (compare new constant values byte-by-byte with every other previously-inserted constant value)

or we pay with additional code-size in the emitted binary

The way it is written now we can safely rip out the two HashMaps and all of this will still work.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:48):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 03 2020 at 19:48):

abrown created PR Review Comment:

@cfallin, I tried adding a WellKnown enum to Amode but I realized that this way is better: 1) if we want to de-duplicate we can, and 2) all of this constant code is co-located, which makes it a little more comprehensible for other developers (a little of it ends up affecting MachBuffer but that is just the easiest way to do things)

Last updated: Nov 22 2024 at 17:03 UTC