Stream: git-wasmtime

Topic: wasmtime / PR #2328 Implement emission of constants

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

abrown edited 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:49):

abrown requested julian-seward1 for a review on PR #2328.

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

abrown requested bnjbvr and julian-seward1 for a review on PR #2328.

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

abrown has marked PR #2328 as ready for review.

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin created PR Review Comment:

Pre-existing, but I think this upward-padding alignment might be written incorrectly; it should be (x + align - 1) & ~(align - 1), not x & ~(align - 1). Eek. (On x64 LabelUse::ALIGN is 1 so no big deal, but...) Could you change this to:

self.island_worst_case_size = (self.island_worst_case_size + I::LabelUse::ALIGN - 1) & !(I::LabelUse::ALIGN - 1);

or better, use an alignment helper function that we must surely already have somewhere?

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin created PR Review Comment:

Yep, this seems fine; it's unlikely we'd have constants smaller than 8 bytes anyway.

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin created PR Review Comment:

I think that the HashMap overhead is reasonable at the "zero large constants" evaluation point, which is where most functions will sit (I think?) except perhaps in SIMD-heavy code. As long as we don't pessimize the common-case compilation, then we're fine, and ~100 bytes of state while we're compiling a function is not bad. So, I'm happy with this -- others feel free to chime in, though :-)

Wasmtime GitHub notifications bot (Nov 04 2020 at 22:45):

cfallin created PR Review Comment:

Hmm, yes, this seems fine; the use-cases for both the static borrow and the inlined/owned data make sense. As a minor design point, folding these two together into a Cow might be a bit more idiomatic: we could have e.g. ConstantDataBytes that is a type-alias to a Cow<'static, [u8]> and then have VCodeConstantData::Value(ConstantDataBytes). Perhaps that's too much type machinery but it would potentially let pass around "owned or borrowed constant bytes" elsewhere.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:17):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:17):

abrown created PR Review Comment:

I didn't find the helper, but I was thinking that if we made CodeOffset an actual struct we could add an align() there. Also, are we sure we don't need to do saturating addition in a few more places in this code?

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:20):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:20):

abrown created PR Review Comment:

Um, I might not be understanding what you mean but I separated WellKnown and Generated because WellKnown could be 'static but Generated can never be 'static because the compiler creates it on the fly (e.g. from a shuffle mask). How can I unify them?

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:20):

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 05 2020 at 21:26):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:26):

cfallin created PR Review Comment:

This is the beauty of std::borrow::Cow! It can hold either a borrow, or an owned thing, and know which is which. So the receiver can get a reference and use the data either way, and the Cow takes care of deallocation if and only if it's owned. A Cow<'static, [u8]> can either be a &'static [u8] or a Box<[u8]> under the hood, and either way can give you a &[u8] to play with.

Anyway, not a huge deal if we don't use this -- it just stood out to me that the Generated and WellKnown variants were basically implementing the same pattern here.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:27):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:27):

cfallin created PR Review Comment:

That's actually a very nice idea!

Re: saturating addition -- perhaps, yes; we should audit this code. I'll take a look.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:33):

abrown submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 21:33):

abrown created PR Review Comment:

Ok, I see it now, and I think I could still make the de-duplication work by inspecting into Cow::Borrowed. Right now I am leaning towards keeping the current three-way separation because it more clearly informs the user of the decision they are making (we need to explicitly choose a variant and one of those, Generated, is not de-duplicated).

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:06):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:06):

cfallin created PR Review Comment:

That's a good point; since this has meaning re: dedup as well I'm fine with the enum as-is. Thanks!

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:07):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Nov 05 2020 at 22:25):

abrown merged PR #2328.

Last updated: Nov 22 2024 at 16:03 UTC