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 fromConstant
toUsedConstant
and another fromUsedConstant
toMachLabel
--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improvegen_constant
.
abrown requested julian-seward1 for a review on PR #2328.
abrown requested bnjbvr and julian-seward1 for a review on PR #2328.
abrown has marked PR #2328 as ready for review.
cfallin submitted PR Review.
cfallin submitted PR Review.
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)
, notx & ~(align - 1)
. Eek. (On x64LabelUse::ALIGN
is1
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?
cfallin created PR Review Comment:
Yep, this seems fine; it's unlikely we'd have constants smaller than 8 bytes anyway.
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 :-)
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 aCow<'static, [u8]>
and then haveVCodeConstantData::Value(ConstantDataBytes)
. Perhaps that's too much type machinery but it would potentially let pass around "owned or borrowed constant bytes" elsewhere.
abrown submitted PR Review.
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 analign()
there. Also, are we sure we don't need to do saturating addition in a few more places in this code?
abrown submitted PR Review.
abrown created PR Review Comment:
Um, I might not be understanding what you mean but I separated
WellKnown
andGenerated
becauseWellKnown
could be'static
butGenerated
can never be'static
because the compiler creates it on the fly (e.g. from a shuffle mask). How can I unify them?
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 fromConstant
toUsedConstant
and another fromUsedConstant
toMachLabel
--in order to make this work ("work" is too strong--"compile"). I'm interested in your feedback before I fix this up and improvegen_constant
.
cfallin submitted PR Review.
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 theCow
takes care of deallocation if and only if it's owned. ACow<'static, [u8]>
can either be a&'static [u8]
or aBox<[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
andWellKnown
variants were basically implementing the same pattern here.
cfallin submitted PR Review.
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.
abrown submitted PR Review.
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).
cfallin submitted PR Review.
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!
cfallin submitted PR Review.
abrown merged PR #2328.
Last updated: Dec 23 2024 at 13:07 UTC