Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NativeAOT: Cover more opcodes in type preinitializer #112073

Merged
merged 11 commits into from
Feb 11, 2025

Conversation

hez2010
Copy link
Contributor

@hez2010 hez2010 commented Feb 2, 2025

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
@MichalStrehovsky
Copy link
Member

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

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.

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

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

@MichalStrehovsky
Copy link
Member

/azp run runtime-nativeaot-outerloop

Copy link

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

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

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@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

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

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

MichalStrehovsky added a commit that referenced this pull request Feb 10, 2025
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.
@hez2010
Copy link
Contributor Author

hez2010 commented Feb 10, 2025

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
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

/azp run runtime-nativeaot-outerloop

Copy link

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
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants