fix(#231): do not emit point ranges for literal constants - #328
Merged
Conversation
Follow-through on a review point I raised on #322 and then merged over. Facts are opt-in, so nothing was broken -- but once a consumer ingests a 322-fact section, dropping to 8 becomes a compatibility conversation. Easier to not emit them than to stop emitting them later. A point range on an operator that IS a literal constant restates what the consumer reads directly off the instruction the fact is keyed to. Measured on a real component core module: 322 facts, of which 314 were exactly this. This section ships into images measured in kilobytes, so a payload that is ~97% restatement of literals is pure cost, and it buries the facts that carry information. Derived point ranges are still emitted: `x & 0` is provably [0,0] on an `i32.and`, and the consumer cannot read that off the operator. That is the interesting case and it survives. Consequence worth stating: "source 1 (constants)" now only fires when a value is provably constant but NOT spelled as a constant. A value the folder rewrote to `i32.const 5` is suppressed -- how it came to be a literal does not matter, only that the emitted operator already states it. Test rework, and one trap avoided: `walk_stops_at_structured_control_flow` had a literal as its only prefix fact, so blanking its expectation to "no facts" would have left a test that passes even if the walk were broken -- weakening a test to fit the code. Its prefix now uses a mask, so it still proves prefix facts ARE produced and post-control-flow ones are NOT. The drop-safety test keeps its load-bearing assertion (the mask fact re-keys 4 -> 2 after DCE removes two instructions); only the suppressed literal entries came out of its expectations. New: `derived_point_range_on_a_non_literal_operator_is_kept` -- the control for the suppression, with ground-truth checking that the fact names the i32.and in the emitted binary. Verified: loom-core --features verification --lib = 504 passed, 0 failed, 2 ignored (no change in count: the suppression removes no test). All 10 facts:: tests green. fmt clean. Refs #231, #303
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-through on a review point I raised on #322 and then merged over. Facts are opt-in so nothing is broken — but once a consumer ingests a 322-fact section, dropping to 8 becomes a compatibility conversation. Easier to not emit them than to stop emitting them later.
The change
A point range on an operator that is a literal constant restates what the consumer reads directly off the instruction the fact is keyed to. Measured on a real component core module: 322 facts, 314 of them exactly this. The section ships into images measured in kilobytes, so a payload that is ~97% restatement of literals is pure cost — and it buries the facts that carry information.
Derived point ranges survive:
x & 0is provably[0,0]on ani32.and, which the consumer cannot read off the operator. That is the interesting case.Consequence, stated plainly: "source 1 (constants)" now only fires when a value is provably constant but not spelled as one. A value the folder rewrote to
i32.const 5is suppressed — how it became a literal doesn't matter, only that the emitted operator already says it.Test rework, and one trap avoided
walk_stops_at_structured_control_flowhad a literal as its only prefix fact. Blanking its expectation to "no facts" would have made it pass while leaving a test that also passes if the walk emits nothing at all — no discriminating power, silently. Its prefix now uses a mask, so it still proves prefix facts are produced and post-control-flow ones are not.The drop-safety test keeps its load-bearing assertion — the mask fact re-keys
value_id 4 → 2after DCE removes two instructions; only the suppressed literal entries left its expectations.New:
derived_point_range_on_a_non_literal_operator_is_kept— the control for the suppression, with ground-truth checking that the fact names thei32.andin the emitted binary.Verified
504 passed / 0 failed / 2 ignored (unchanged count — the suppression removes no test), all 10
facts::tests green, fmt clean.Refs #231, #303