Skip to content

Comments

NativeAOT: Cover more opcodes in type preinitializer#112073

Merged
MichalStrehovsky merged 11 commits intodotnet:mainfrom
hez2010:more-interpreter
Feb 11, 2025
Merged

NativeAOT: Cover more opcodes in type preinitializer#112073
MichalStrehovsky merged 11 commits intodotnet:mainfrom
hez2010:more-interpreter

Conversation

@hez2010
Copy link
Contributor

@hez2010 hez2010 commented Feb 2, 2025 •
edited
Loading

Some low hanging fruits.
I initially did this to allow preinitializing method call to Math.*.
But it doesn't harm to support all of conv opcodes, so I did for them all.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 2, 2025
This was referenced Feb 2, 2025
Open
Open
@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 3, 2025

But it doesn't harm to support all of conv opcodes, so I did for them all.

It doesn't harm, but each of these requires test coverage. We keep our tests here:

https://github.com/dotnet/runtime/tree/main/src/tests/nativeaot/SmokeTests/Preinitialization

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 3, 2025

I'll be OOF for several days so tests will be added later. But feel free to take it if you would like.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 3, 2025

I'll be OOF for several days so tests will be added later. But feel free to take it if you would like.

It can wait.

@MichalStrehovsky MichalStrehovsky added the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 3, 2025
@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 5, 2025
@hez2010
Copy link
Contributor Author

hez2010 commented Feb 6, 2025

@MichalStrehovsky PTAL.

MichalStrehovsky
MichalStrehovsky approved these changes Feb 7, 2025
View reviewed changes
Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for adding the tests!

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 8, 2025

/ba-g wasm failure is already fixed in main #112244

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 8, 2025

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 8, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 8, 2025

Hmm the linux-arm failure seems related. Is there anything special on arm32?

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 8, 2025

Hmm the linux-arm failure seems related. Is there anything special on arm32?

I don't actually know how we define float conversion rules. I'd not be surprised if it's float precision issues.

The reasons why this wasn't implemented were twofold: nothing I tried ever needed it (we go for 20% effort, 80% effect), and i didn't have confidence in implementing these.

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 9, 2025

I added some messages for debug. Can you trigger nativeaot-outerloop again?

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 9, 2025

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 9, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@build-analysis build-analysis bot mentioned this pull request Feb 9, 2025
Open
3 tasks
@hez2010
Copy link
Contributor Author

hez2010 commented Feb 9, 2025 

Unhandled exception. System.Exception: TestConversions+NativeIntConversions is not preinitialized.

Seems that we handled the conversion correctly. (I actually checked ECMA-335 for the fix d3b2b0f so this should not be a problem)
It's just that the type didn't get preinitialized due to some reasons. Does this ring a bell?

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 9, 2025 •
edited
Loading

Maybe it's the same issue I addressed in 6c63ed8 before, where we also need to handle neg as the test cases use -42 which has a negative sign.
I would like to request another nativeaot-outerloop challenge.

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 10, 2025

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 10, 2025

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky added a commit that referenced this pull request Feb 10, 2025
@MichalStrehovsky
Do not run NAOT arm64 OSX testing on all PRs
f3a9562 
These legs have been constantly timing out since beginning of January because Helix is overloaded (e.g. #112073). In looking at why we don't see timeouts with CoreCLR, I found out we do not test ARM64 OSX with CoreCLR on all PRs. So this moves it to the same plan as e.g. ARM32 Linux.
@MichalStrehovsky MichalStrehovsky mentioned this pull request Feb 10, 2025
Merged
@hez2010
Copy link
Contributor Author

hez2010 commented Feb 10, 2025 •
edited
Loading

I just used the alt jit to spot the issue, and it turns out to be:

Could not preinitialize '[Preinitialization]TestConversions+NativeIntConversions': Method '[S.P.CoreLib]System.Decimal.op_Implicit(int64)', opcode 'newobj' Align8

Seems like a cursed test case as I don't plan to support decimal.

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 10, 2025

Test should pass now.

jkotas pushed a commit that referenced this pull request Feb 10, 2025
@MichalStrehovsky
Do not run NAOT arm64 OSX testing on all PRs (#112342)
91bbb32 
These legs have been constantly timing out since beginning of January because Helix is overloaded (e.g. #112073). In looking at why we don't see timeouts with CoreCLR, I found out we do not test ARM64 OSX with CoreCLR on all PRs. So this moves it to the same plan as e.g. ARM32 Linux.
@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Feb 10, 2025

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 10, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@hez2010
Copy link
Contributor Author

hez2010 commented Feb 11, 2025

nativeaot-outerloop is passing.

@MichalStrehovsky MichalStrehovsky merged commit 8ae8594 into dotnet:main Feb 11, 2025
115 of 120 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 12, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
21f8a18 
* main:
  [Android] Run CoreCLR functional tests on Android (dotnet#112283)
  [LoongArch64] Fix some assertion failures for Debug ILC building Debug NativeAOT testcases. (dotnet#112229)
  Fix suspicious code fragments (dotnet#112384)
  `__ComObject` doesn't support dynamic interface map (dotnet#112375)
  Native DLLs: only load imported DLLs from System32 (dotnet#112359)
  [main] Update dependencies from dotnet/roslyn (dotnet#112314)
  Update SVE instructions that writes to GC regs (dotnet#112389)
  Bring up android+coreclr windows build.  (dotnet#112256)
  Never use heap for return buffers (dotnet#112060)
  Wait to complete the test before releasing the agile reference. (dotnet#112387)
  Prevent returning disposed HTTP/1.1 connections to the pool (dotnet#112383)
  Fingerprint dotnet.js if writing import map to html is enabled (dotnet#112407)
  Remove duplicate definition of CORECLR_HOSTING_API_LINKAGE (dotnet#112096)
  Update the exception message to reflect current behavior. (dotnet#112355)
  Use enum for frametype not v table (dotnet#112166)
  Enable AltJits build for LoongArch64 and RiscV64 (dotnet#110282)
  Guard members of MonoType union & fix related bugs (dotnet#111645)
  Add optional hooks for debugging OpenSSL memory allocations (dotnet#111539)
  JIT: Optimize struct parameter register accesses in the backend (dotnet#110819)
  NativeAOT: Cover more opcodes in type preinitializer (dotnet#112073)
@hez2010 hez2010 mentioned this pull request Mar 4, 2025
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@MichalStrehovsky MichalStrehovsky MichalStrehovsky approved these changes

Assignees

No one assigned

Labels

area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hez2010 @MichalStrehovsky