Skip to content

Conversation

lewing
Copy link
Member

@lewing lewing commented Apr 25, 2025

Implement a PackedSimd vectorized version of Hexconverter.TryDecodeFromUtf16

Then fix several things that were broken in the AOT intrinsics to make that optimization work.

@ghost ghost added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 25, 2025
@lewing lewing requested review from kg and radekdoulik April 25, 2025 20:37
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

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

Looks right, with the caveat that I didn't test the new implementation myself

@lewing lewing marked this pull request as ready for review April 26, 2025 01:41
@lewing
Copy link
Member Author

lewing commented Apr 26, 2025

/ba-g coreclr failures are not related

@lewing
Copy link
Member Author

lewing commented Apr 26, 2025

Looks right, with the caveat that I didn't test the new implementation myself

the select mask was swapped but the tests caught it.

@lewing lewing removed the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 26, 2025
@lewing
Copy link
Member Author

lewing commented Apr 27, 2025

Looks like there are some regressions with invalid values, taking a look.

@lewing
Copy link
Member Author

lewing commented Apr 27, 2025

It looks like the tests are only failing in windows without aggressive trimming, in CI, but not locally. ugh

@lewing
Copy link
Member Author

lewing commented Apr 28, 2025

@radekdoulik it looks like the failing paths are on the AOT builds

@radekdoulik
Copy link
Member

@radekdoulik it looks like the failing paths are on the AOT builds

I have no luck replicating it locally. Tried both Release and Debug configurations.

I was running EMSDK_PATH=/Users/rodo/git/runtime/src/mono/browser/emsdk ./dotnet.sh build -c Debug /t:test /p:_WasmAllowAOTDebug=true /p:EnableAggressiveTrimming=true /p:RunAOTCompilation=true /p:TargetOS=browser src/libraries/System.Runtime/tests/System.Runtime.Tests and similar for Release.

@lewing
Copy link
Member Author

lewing commented Apr 29, 2025

Passed locally with AOT on windows using the same version of chrome. I wonder if this is another msvc error in the cross compiler (see: #114786)

 info: Finished:    System.Runtime.Tests.dll
  info: Stored D:\dotnet\runtime\artifacts\bin\System.Runtime.Tests\Release\net10.0-browser\browser-wasm\AppBundle\xhar
  ness-output\testResults.xml results 17698363 bytes
  info: Finished uploading 17698363 bytes of RESULTXML
  info: Xml file was written to the provided writer.
  info:
  info: === TEST EXECUTION SUMMARY ===
  info: Tests run: 64996 Passed: 64882 Inconclusive: 0 Failed: 0 Ignored: 0 Skipped: 114
  info:
  info: test-main.js exiting WasmTestRunner.dll System.Runtime.Tests.dll with result 0 and linear memory 300154880 byte
  s
  info: MONO_WASM: forceDisposeProxies done: 14 imports, 0 exports, 0 GCHandles, 0 JSHandles.
  info: WASM EXIT 0
  info: Waiting to flush log messages with a timeout of 120 secs ..
  info: Closing 1 browser tabs before setting the main tab to config page and quitting.
  XHarness exit code: 0
  ----- end Tue 04/29/2025 17:03:46.84 ----- exit code 0 ----------------------------------------------------------
  XHarness artifacts: D:\dotnet\runtime\artifacts\bin\System.Runtime.Tests\Release\net10.0-browser\browser-wasm\AppBund
  le\xharness-output

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:14:30.11

@lewing
Copy link
Member Author

lewing commented Apr 29, 2025

cc @akoeplinger

@lewing
Copy link
Member Author

lewing commented May 6, 2025

I've extracted out some of the marginally related changes. What is left in the pr now is:

  • a fix for PackedSimd.BitwiseSelect argument ordering
  • removing OP_WASM_ONESCOMPLEMENT and using the common path for OP_ONES_COMPLEMENT now that llvm is fixed
  • additional tests
  • The original optimization (aka the first two commits)

I'm happy to split out the fixes completely from the optimization but I think it is reviewable as is.

@lewing lewing removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label May 6, 2025
@lewing lewing requested a review from kg May 6, 2025 16:10
@lewing
Copy link
Member Author

lewing commented May 6, 2025

/ba-g failures are not related

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants