bnjbvr commented on issue #4551:
Thanks all for the first batch of reviews!
@cfallin this makes sense to me. At this point there's not much that needs those relocations, if I'm not mistaken; mostly things that weren't trivially
Hash
ables, or source locations that were absolute, or external names being embedded and copied around. I'm looking into this.
bnjbvr commented on issue #4551:
I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :
- fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
- there's a proliferation of
Name
types that could potentially be unified, maybe?ExternalName
could just become a reference into theFunctionParameters
mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. SoExternalName
would become a new entity type, and there'd beExternalNameData
in the table. Might not be true for libcalls which identity may matter during codegen, so may require thatExternalName
becomes a sum type: either a reference into theFunctionParameter
's table, or theLibcall
embedded itself.- optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).
bnjbvr edited a comment on issue #4551:
I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :
- fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
- there's a proliferation of
Name
types that could potentially be unified, maybe?ExternalName
could just become a reference into theFunctionParameters
mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. SoExternalName
would become a new entity type, and there'd beExternalNameData
in the table. Might not be true for libcalls which identity may matter during codegen, so may require thatExternalName
becomes a sum type: either a reference into theFunctionParameter
's table, or theLibcall
embedded itself.- right now, if a
UserExternalNameRef
changes (that is, a function calls decides to change callee), then it changes the identity of theFunctionStencil
, thus resulting in a cache miss; this could likely be refactored and improved, enabling more cache hits.- optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).
bnjbvr edited a comment on issue #4551:
I've updated the initial text of this PR, and while not ideal/perfect, I claim this is ready for review and could be merged as a first step. Remaining things to do as follow-ups, either myself in my copious free time, or other interested contributors :heart_eyes: :
- fuzzing could be improved, to generate an instruction that could serve as replacement of another instruction. This would likely also require a position where to put this instruction, and some care ensuring that the instruction is compatible (same number of input/returns + same types for all inputs/returns)
- there's a proliferation of
Name
types that could potentially be unified, maybe?ExternalName
could just become a reference into theFunctionParameters
mapping of reference to actual name value, as (it's likely, not proven that) codegen doesn't care about the identity of the function callee. SoExternalName
would become a new entity type, and there'd beExternalNameData
in the table. Might not be true for libcalls which identity may matter during codegen, so may require thatExternalName
becomes a sum type: either a reference into theFunctionParameter
's table, or theLibcall
embedded itself.- right now, if a
UserExternalNameRef
changes (that is, a function call switches the callee identity), then it changes the identity of theFunctionStencil
, thus resulting in a cache miss; this could likely be refactored and improved, enabling more cache hits.- optimization work could be done to identify why a recompilation is, in the worst case, only 50% faster than compiling from scratch (it should be much higher in theory).
bnjbvr commented on issue #4551:
@cfallin This new version incorporates changes to address your review feedback during the live review session (that we organized to help going through the changes, as this is a large patch). This is now ready and passes
cargo test --all
on my machine, with no performance regression on compile times, so I would claim this is ready :-) Cheers!
cfallin commented on issue #4551:
I just merged in
main
to this PR to see if it fixes CI.
cfallin commented on issue #4551:
Hmm, @bnjbvr it looks like some of the changes broke the fuzz targets -- happy to merge once that is fixed up.
Last updated: Dec 23 2024 at 12:05 UTC