Skip the balanced switch dispatch optimization for patterns on floating-point inputs #62322
Conversation
This file contains 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
333fred
approved these changes
Jul 5, 2022
| @@ -10177,162 +10171,156 @@ static int M(float d) | |||
| expectedOutput: expectedOutput); | |||
| compVerifier.VerifyIL("C.M", @" | |||
| { | |||
| // Code size 388 (0x184) | |||
| // Code size 374 (0x176) | |||
There was a problem hiding this comment.
I find it very interesting that the IL is smaller in both of the modified tests. Do we understand that?
|
|
||
| if (t1.Input.Type.SpecialType is SpecialType.System_Double or SpecialType.System_Single) | ||
| { | ||
| // The optimization (using balanced switch dispatch) breaks the semantics of NaN |
There was a problem hiding this comment.
nit: consider linking to the issue. up to you.
From offline discussion: consider doing an upfront nan check against the input to jump to the 'catch-all' case. If the value is not nan, then run the value through the balanced switch case. |
RikkiGibson
approved these changes
Jul 6, 2022
jcouv
changed the title
ghost
added this to the
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Jul 7, 2022
* upstream/main: (62 commits) Prevent assert from being hit (dotnet#62366) Don't offer '??=' for pointer types (dotnet#62476) Integrate generator times into /reportAnalyzer (dotnet#61661) Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373) Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444) Update PublishData.json Complete 'file' support for `SyntaxGenerator` (dotnet#62413) Apply changes directly to text buffer (dotnet#62337) Remove LangVer check from extended nameof binding (dotnet#62339) Fixed shared project file error (dotnet#62466) Handle new error codes Use MSBuid generated property for package path Exclude BCL libraries from Roslyn vsix Bump the integration test timeouts a bit Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322) Test helpers shouldn't escape quotes by default (dotnet#62321) Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320) Install 3.1 runtime for SBOM tool Generate VS SBOM during official build. Minor refactoring in 'required' handling for override completion (dotnet#62422) ...
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 #62241
The DAG for the scenario
< -40.0 => "Too low", >= -40.0 and < 0 => "Low", ..., double.NaN => "NaN"has a test fort0 < -40at the top-level. When that test is false, the next test ist0 >= -40. When that test is false, we're in the NaN case (NaN is the only possible remaining value so we don't need an explicit test). That DAG is correct.Original DAG:
But the lowering of DAGs can take multiple relational tests on a given input and convert it into a balanced switch dispatch.
That optimization results in a top-level
< 0test. The< -40.0and>= -40.0tests are therefore moved under thetruecase of that top-level test, and the<= 10test is under thefalsecase. That is more balanced, but it is incorrect because a NaN input value yields false for any comparison. In that configuration, the path to the NaN case (marked with dotted line) is impossible.Balanced switch dispatch generated by optimization:
I haven't found a way to keep the benefits of the optimization while maintaining proper NaN semantics, so this PR disables the optimization for inputs of floating-point types.
Filed #62390 to explore re-enabling some partial-but-safe optimization.