bnjbvr commented on Issue #2258:
would it make sense to hold the EmitInfo in the VCode container itself, and plumb it into the instruction emission from there?
I'm happy to do this, but that would break the symmetry with
EmitState
... which could also live in Vcode in fact. Maybe also moving EmitState would help reducing the traits complexity... Any preference here? (Happy to chat about this on Zulip)
cfallin commented on Issue #2258:
I suppose I see the difference between
EmitState
andEmitInfo
as: the vcode should be emittable multiple times (the instructions are immutable during emit), so we wantEmitState
to be an external thing -- it's transient, per emission; whileEmitInfo
encapsulates some information that is tied to the lowering and should not be changed, so its lifetime should be as long as the VCode. (And given that it shouldn't change, there is really only one "correct value" to use when emitting a given VCode body, so it doesn't seem right to require it as a parameter.) So from that perspective, the asymmetry doesn't bother me too much. That said, I'm happy to go with your original design as well if you prefer it :-)
cfallin edited a comment on Issue #2258:
I suppose I see the difference between
EmitState
andEmitInfo
as: the VCode should be emittable multiple times (the instructions are immutable during emit), so we wantEmitState
to be an external thing -- it's transient, per emission; whileEmitInfo
encapsulates some information that is tied to the lowering and should not be changed, so its lifetime should be as long as the VCode. (And given that it shouldn't change, there is really only one "correct value" to use when emitting a given VCode body, so it doesn't seem right to require it as a parameter.) So from that perspective, the asymmetry doesn't bother me too much. That said, I'm happy to go with your original design as well if you prefer it :-)
bnjbvr commented on Issue #2258:
That's a good argument, thanks! I hadn't thought about the fact that we could emit several times (I wonder what such a use case would be though, since an emitter who'd like to emit in several places would emit in a buffer, and then would carry a cheap copy rather than the full-blown emission.) I'll rework my patch.
bnjbvr commented on Issue #2258:
@cfallin Updated the patch, please let me know if you want to take another look!
Last updated: Dec 23 2024 at 12:05 UTC