Repository navigation
Preserve explicit Not methods during boolean simplification - #582
Merged
Merged
Conversation
Owner
|
good to merge when conflicts are resolved |
Do not interpret a method-based Not as built-in negation or rewrite it through equality during conditional simplification. Preserve the selected method and its observable effects using the existing unary emitter. Standard and Light regressions cover constant and parameter operands, conditional branches, ignored results and nullable method arguments, with interpreter optimization enabled and disabled. Focused tests pass on net8.0, net10.0 and net472; the unfixed compiler changes results and omits calls. Full solution builds, legacy suites and Framework runners pass. Existing master MultiplyChecked parity failures reproduce identically on net8.0/net10.0.
jogibear9988
force-pushed
the
fix/custom-not-simplification
branch
from
November 4, 2026 19:20
e6baeac to
402fd81
Compare
jogibear9988
added a commit
to jogibear9988/FastExpressionCompiler
that referenced
this pull request
Nov 4, 2026
The explicit Not implementation and its duplicate fixture now come from merged dadhi#582. Retain only the separate lifted complement and boxed identity edge cases.
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.
Boolean interpretation and conditional simplification can treat a Not node with an explicitly selected method as built-in negation. This changes the returned value and can omit the selected method call and its observable effects.
Add Method-null guards to the two existing simplification sites. Method-based nodes continue through the existing unary emitter. The implementation adds three lines and changes one condition; no new emitter or traversal.
Directly based on upstream master
65ef6641cbe709624d713b1db5248c4b877fbd13; one commit (402fd812), independent of #558 and the other standalone PRs.Correctness and validation
Four shared methods exercise constant and parameter operands, conditional shapes, ignored-result effects, nullable arguments, and interpreter optimization enabled/disabled. Standard and LightExpression are checked against System, including method call counts.
The unfixed .NET 10 regression reports wrong results and missing method calls in both APIs; the same regression passes after the patch.
Focused Standard and LightExpression regressions pass on .NET 8, .NET 10 and .NET Framework 4.7.2 (4 registered methods per API).
Full Release solution build passes with zero warnings/errors.
Legacy native suites pass: 1,756 cases on .NET 10 and 1,755 on .NET 8.
Framework runner passes 202 TestX + 1,639 legacy cases.
The full .NET 8/10 runner still reports the existing nullable MultiplyChecked parity failure for (1,1) and (2,1) in both APIs. These are the previously documented baseline failures; the rebased branch reports the same failing method and inputs. The baseline was not rerun in this conflict-resolution pass. No regression assertions were skipped or weakened. Legacy suites were invoked separately because the existing TestX failure gates the normal runner.
Rebase validation (2026-11-04)
Rebased onto freshly fetched upstream master, retaining both master's
RuntimeTypeConstantRegressionTestsregistrations and this PR'sMethodBasedNotRegressionTestsregistrations. The two production guards are unchanged; the PR remains one independent commit.Performance was not remeasured for this registration-only conflict resolution. Earlier tables used the previous master revision and have been removed to avoid presenting them as measurements against the current base. No new timing or allocation claim is made here.