Repository navigation
Fix shared values in conditional code in the PostScript compilers - #22418
Open
zerotohero3 wants to merge 2 commits into
Open
zerotohero3 wants to merge 2 commits into
zerotohero3 wants to merge 2 commits into
Conversation
A value used more than once is stored, in a Wasm local or in a JS tmp
slot, where it's compiled for the first time. When that happens in a
branch of a ternary node, the value is never stored if the other branch
is taken, so it's read as 0 (or as the value of another node reusing the
local) with Wasm, and as whatever a previous call left in the slot with
JS. For example `{ dup sin exch 0 gt { 1 add } if }` returns 0 instead of
`in0 sin` for any non-positive input.
Compute the shared values used in the branches before the condition,
which is fine since the operators have no side effects.
The fast paths in `_compileNodeAsBoolI32` compile the operands of a comparison, or of a boolean operator, directly, so a condition shared by several ternary nodes was compiled for each of them. The remaining use count of its operands was then decremented too many times, hence their local could be released, and reused for another value, while still needed.
This branch has not been deployed
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
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.
Fixes #22416
_compileNodeAsBoolI32compiled its operands for each use, which decremented their use count too many times, so their local could be reused for another value while still needed.Besides the new tests, I checked the Wasm/JS compilers against the interpreter on randomly generated functions and on all the Type 4 functions in
test/pdfs: with this patch and #22417, the only remaining differences are for0 0 atanwith negative zeros, which is an error in PostScript anyway.