Ecma edit for conv.ovf.<to type>.un.
#56450
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sandreenko
removed
the
area-CodeGen-coreclr
|
@sandreenko I'd like to have a discussion here about the behavior of conversion of floating point numbers to integers where the conversion is outside of the range of the destination integer type. We have had customer reports of problems due to inconsistencies here between behavior on x86/x64/arm/arm64, and I'd like for us to revisit what we do here. I did some research last year, and my personal preference is to follow the arm64 model of conversions long term, but that is a possibly breaking change everywhere. |
Yes, I remember that and your test with different models, the new test added in this PR will need to be updated once the |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
looks like this test exposes an unknown issue on arm64 Linux/Osx. Looking. |
sandreenko
force-pushed
the
ecmaCastOvfUn
branch
from
0b53d4c to
bc184e9
Compare
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I have disabled the bad Jit optimization for this test and the PR should be ready for merge. |
| Edit rule 12 to specify that "The method signature defined by *MethodBody* shall match those defined by *MethodDeclaration* exactly if *MethodDeclaration* defines a method on an interface or be *covariant-return-compatible-with* (§I.8.7.1) if *MethodDeclaration* represents a method on a class." | ||
|
|
||
| ### III.3.19, conv.ovf.to type.un (page 354) |
There was a problem hiding this comment.
This should be put into a new section describing feature area. Effectively, each set of changes to the Ecma standard describes what the general impact is, and then there is the modification to the actual standardese. As it is this work is appended onto the covariant return types work, but really should be in its own chunk.
There was a problem hiding this comment.
I see, I have added a new chapter and tried to add a justification for the change. I have not found a template for this, looks like different chapters are organized differently, please tell me if you want me to add issue numbers, link to the current standard, or something else.
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <CLRTestPriority>1</CLRTestPriority> |
There was a problem hiding this comment.
@sandreenko I think you've already taken note of this, but I believe the reason CI has been green so far is because the outerloop has only been triggered for CoreCLR, and not Mono. Not sure what is the best course of action here.
There was a problem hiding this comment.
yes, thanks for reminding me. I have not found a pipeline that runs outerloop on mono. Do we run them there nowadays?
…ger_proxy_attribute * origin/main: disable token info in traces. (dotnet#56780) [debugger] Fix debugger.break behavior (dotnet#56788) [mono][wasm] Allow setting env variables with '=' characters in the test runner. (dotnet#56802) Ecma edit for `conv.ovf.<to type>.un`. (dotnet#56450) Mark HandleProcessCorruptedStateExceptionsAttribute as obsolete (dotnet#56664) Enable SxS install of previews on Mac OS (dotnet#56797) CoreCLR runtime tests + Mono on the x64 iOS simulator (dotnet#43954) [main] Update dependencies from mono/linker (dotnet#56593) STJ: Fix deserialization of UInt16 properties (dotnet#56793)
ghost
locked as resolved and limited conversation to collaborators
Edit ECMA description for ovf.un conversion when the source is a floating-point type.
Add a test for this conv table parts:
Question: Have not we had a conformance test for it already?
I have found only a small coverage in https://github.com/dotnet/runtime/tree/main/src/tests/JIT/IL_Conformance/Old/Conformance_Base without tests for
.ovfandovf.un.Closes #53189, the test will probably fail on mono. I will disable it there after CI confirms it.