jameysharp opened PR #4984 from ssa-cleanup
to main
:
This is a collection of all the cleanups I set aside while working on #4955 and #4966. According to DHAT and hyperfine, these patches together have almost no effect on performance. DHAT says this reduces total memory allocated, max heap size, and bytes written, while increasing instructions retired and bytes read, but all by tiny fractions of a percent.
jameysharp requested cfallin for a review on PR #4984.
cfallin submitted PR review.
cfallin submitted PR review.
cfallin created PR review comment:
I think this results in a change in behavior with respect to before: earlier we had an assert that ensured a block could only be sealed once (line 437 in the original); now we silently accept
Yes
here. That is probably OK -- the effect is nothing if already sealed -- but I just want to make sure we've thought about this and that it doesn't contradict docs elsewhere?
cfallin created PR review comment:
I wonder: is this actually necessary?
SecondaryMap
(or at least, one created with::with_default()
) will return a readonly borrow of some shared "default" value for anIndex
of a non-existing index, or will dynamically expand as needed for anIndexMut
access; the lack of prior expansion is unobservable (as opposed to just-in-time expansion), I think. Or is that not the case?
jameysharp created PR review comment:
In terms of public API, the behavior is unchanged. I moved the assert to
seal_block
; andseal_all_blocks
was only callingseal_one_block
if the block was not already sealed.Making the internal helpers just no-op for sealed blocks meant that I could remove a conditional from
seal_all_blocks
and check only once inmark_block_sealed
instead.As far as private callers go,
append_jump_argument
callsmark_block_sealed
but only on a block it just created, so it definitely isn't sealed.
jameysharp submitted PR review.
jameysharp submitted PR review.
jameysharp created PR review comment:
I almost had myself convinced that I could delete
declare_block
entirely, because as you say, the allocated size of aSecondaryMap
is unobservable... almost.The exception is
SecondaryMap::keys
, which iterates only over indexes that are currently initialized. Andseal_all_blocks
usesself.ssa_blocks.keys()
to determine which blocks need to be sealed.So if I don't ensure that the map has grown at least to the declared block's index, then I think there's a sequence of public API calls against a
FunctionBuilder
that would hit a debug assertion. It's something likecreate_block
,switch_to_block
,ensure_inserted_block
, andfinalize
, without declaring any predecessors or callinguse_var
.I didn't specifically test that because it was starting to make my head hurt. So maybe it's actually okay, but I didn't want to think about it further.
jameysharp updated PR #4984 from ssa-cleanup
to main
.
jameysharp merged PR #4984.
Last updated: Dec 23 2024 at 13:07 UTC