Stream: git-wasmtime

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


view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 18:57):

abrown requested cfallin for a review on PR #2328.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 27 2020 at 22:44):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 29 2020 at 22:59):

cfallin submitted PR Review.

view this post on Zulip 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 :-)

view this post on Zulip 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?

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 19:21):

abrown submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown submitted PR Review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 19:45):

abrown submitted PR Review.

view this post on Zulip 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:

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 03 2020 at 19:48):

abrown submitted PR Review.

view this post on Zulip 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: Dec 23 2024 at 12:05 UTC