Refactoring the generic-math CreateChecked/Saturating/Truncating APIs to match API review #69756
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
…cating, and TryCreate implementations
…ecked, Saturating, and Truncating
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
dakersnar
reviewed
May 24, 2022
dakersnar
approved these changes
May 24, 2022
There was a problem hiding this comment.
It seems like most of this "if it compiles it's correct" type of code. Testing should validate the small amounts of logic present, mostly in the Saturating code. Additionally, anywhere where you have introduced new explicit converters should be tested (Half, Int128, NFloat, BigInteger, Complex, etc). The CreateChecked tests look good, assuming you add CreateSaturating and CreateTruncated later, this looks good to me!
…e primitive types
|
CC. @MichalStrehovsky, @jeffhandley. NativeAOT seems to be unhappy with something here. I'd guess its due to the first use of |
|
Seems the compilation failures are a bit more widespread. CC @trylek and @davidwrighton This is the first PR using the |
|
Likewise seems that Mono is doing odd things with |
|
The main issue looks to be: You can likewise see the below, where and another one: |
|
There are some Mono failures that look to be Mono related. There are some 32-bit failures that look like an error in the test logic I wrote. |
It's unhappy about the INumberBase.CreateChecked. It's a default interface method. I didn't realize we have static default interface methods (I thought that's what the WIP pull request #66887 is about?). I'm looking into it. Does this need to go in Preview 5? |
During the last status sync it was indicated that things should be generally working with just a few remaining edge cases. Apparently those edges are a bit more visible than expected
The plan was for this to be in P5 yes. Worst case I'll need to remove the DIM and manually declare the implementation for all 20 types. |
I've done this with baf69de. You'll want to remember to revert this if trying to repro the failures above |
|
@MichalStrehovsky there's another ILC failure. It's unclear what is causing this one: |
tannergooding
force-pushed
the
generic-math
branch
from
cf9db31 to
7f30dc3
Compare
That one is crossgen2 ( |
Is there a good way to debug into this. It looks like an opaque MSBuild task and not some command line I can just run with an attached debugger. |
@dotnet/crossgen-contrib might help. But it looks like the task dumps a response file somewhere and invokes crossgen2 with the response file. |
|
Crossgen2 seems to be unhappy with some of the existing I've removed the attributes. |
jkotas
reviewed
May 25, 2022
We should get an issue opened on this if you plan to merge this workaround. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
tannergooding
force-pushed
the
generic-math
branch
from
2c07b35 to
0bea61d
Compare
|
/azp run |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
tannergooding
commented
May 27, 2022
src/coreclr/vm/jithelpers.cpp
Outdated
Comment on lines
603
to
604
| // specially handle NaN otherwise we return (Int64.MaxValue + 1) | ||
| ret = 0; |
There was a problem hiding this comment.
This was being incorrectly handled resulting in invalid results for Arm32.
This entire method could likely be simplified to just (UINT64)val like I had prototyped in #61761, but it should be handled in a separate PR.
I'd like to actually get #61761 completed and in, but its likely too late in the cycle for .NET 7 to take such a big "break". It was effectively "done" other than the Mono work and perf cleanup to avoid calls on x64.
There was a problem hiding this comment.
Reverted this in favor of just explicitly checking for NaN and returning 0 in the relevant APIs to simplify code churn for the time being.
tannergooding
force-pushed
the
generic-math
branch
from
aef461e to
f95f854
Compare
|
Failure is unrelated: #60705 |
|
/backport release/7.0-preview5 |
|
/backport to release/7.0-preview5 |
|
Started backporting to release/7.0-preview5: https://github.com/dotnet/runtime/actions/runs/2416671628 |
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The git-diff is pretty bad so it might be easier to review this "by commit".