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

Adding vectorized implementations of Log2 to Vector64/128/256/512 #96455

Merged
merged 8 commits into from
Jan 11, 2024

Conversation

tannergooding
Copy link
Member

This makes progress towards #93513 and sets up the basic infrastructure for testing the new APIs.

In general the tests are following the same logic as the scalar versions and testing the same values, for simplicity.

The algorithm is the same as being used for the TensorPrimitives type but isn't currently shared due to that being out of band and therefore not supporting the ISimdVector interface since it needs to multi-target $(NetCoreAppPrevious). It's likely not worth the additional #ifdef at this time to have $(NetCoreAppCurrent) calling the APIs exposed on the vector types.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, 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.

@ghost
Copy link

ghost commented Jan 3, 2024

Tagging subscribers to this area: @dotnet/area-system-numerics
See info in area-owners.md if you want to be subscribed.

Issue Details

This makes progress towards #93513 and sets up the basic infrastructure for testing the new APIs.

In general the tests are following the same logic as the scalar versions and testing the same values, for simplicity.

The algorithm is the same as being used for the TensorPrimitives type but isn't currently shared due to that being out of band and therefore not supporting the ISimdVector interface since it needs to multi-target $(NetCoreAppPrevious). It's likely not worth the additional #ifdef at this time to have $(NetCoreAppCurrent) calling the APIs exposed on the vector types.

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@stephentoub
Copy link
Member

The algorithm is the same as being used for the TensorPrimitives type but isn't currently shared due to that being out of band

Presumably we should we be adding a net9.0 TFM to that package and specializing the implementation for net9.0 to use these Vector64/128/256/512 APIs, right?

@tannergooding
Copy link
Member Author

Presumably we should we be adding a net9.0 TFM to that package and specializing the implementation for net9.0 to use these Vector64/128/256/512 APIs, right?

We have that currently, it's what $(NetCoreAppCurrent) is targeting. We certainly could consume these APIs, but it's going to involve additional ifdefs in many of the algorithms and it's not clear if that's worth it since we need to duplicate the logic regardless currently.

No strong preference and happy to leave it as is or update it to:

public static Vector128<T> Invoke(Vector128<T> x)
{
#if NET9_0_OR_GREATER
    if (typeof(T) == typeof(double))
    {
        return Vector128.Log2(x.AsDouble()).As<double, T>();
    }
    else
    {
        Debug.Assert(typeof(T) == typeof(float));
        return Vector128.Log2(x.AsSingle()).As<float, T>();
    }
#else
    if (typeof(T) == typeof(double))
    {
        return Log2OperatorDouble.Invoke(x.AsDouble()).As<double, T>();
    }
    else
    {
        Debug.Assert(typeof(T) == typeof(float));
        return Log2OperatorSingle.Invoke(x.AsSingle()).As<float, T>();
    }
#endif
}

@stephentoub
Copy link
Member

No strong preference and happy to leave it as is or update it to:

Thanks. I'd prefer to use the new APIs when targeting .NET 9, in order to a) help validate them (all of the TensorPrimitives tests will accrue to the Vector APIs), b) benefit from any improvements that might be possible in those core APIs because they're lower in the stack, c) be able to better keep the implementations in sync between what's in TensorPrimitives for .NET 8 and what's in these Vector APIs for 9, and d) reduce the binary size a bit for the library deployed for .NET 9. Eventually, when we no longer need to build for <= .NET 8, we could also remove the custom implementations.

This could also be our answer to #96452 if that's desirable, e.g. these would only be accelerated for .NET 9.

@tannergooding
Copy link
Member Author

One of the failures is a JIT assert:

Assert failure(PID 10027 [0x0000272b], Thread: 85988 [0x14fe4]): Assertion failed '!m_inited' in 'System.Runtime.Intrinsics.VectorMath:Log2Single[System.Runtime.Intrinsics.Vector128`1[float],System.Runtime.Intrinsics.Vector128`1[int],System.Runtime.Intrinsics.Vector128`1[uint]](System.Runtime.Intrinsics.Vector128`1[float]):System.Runtime.Intrinsics.Vector128`1[float]' during 'Importation' (IL size 976; hash 0xefb066b7; Tier0)

}
}

#if !NET9_0_OR_GREATER
Copy link
Member

Choose a reason for hiding this comment

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

Is the test project set up to run tests against .NET 8, or are we only testing the .NET 9 and netstandard2.0 bits right now? Given the non-trivial code differences involved in all three versions, it'd probably be best if we ran all three sets of tests... I think right now we're only running two, at least locally (maybe somehow CI runs all of them?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm unsure about which it runs currently. I agree it'd be beneficial if all are running, even if some are in the outer loop.

Copy link
Member

Choose a reason for hiding this comment

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

@ericstj, @ViktorHofer, I understand from Viktor that this is currently not possible. We're going to have a large amount of code in this library that differs between its netstandard, net8, and net9 assets, and really need to run the tests on all three. How can we make that happen in some form?

Copy link
Member

Choose a reason for hiding this comment

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

Steve and I had a conversation about this offline. Being able to test previous .NETCoreApp versions is tracked via #54639.

here are couple things that would need to change:
Obviously, all referenced projects / packages need to support that older .NETCoreApp TFM. I.e. RemoteExecutor currently doesn't (for no particular reason). I have a PR open that changes that: Update Arcade TFMs according to TFM docs by ViktorHofer · Pull Request #14332 · dotnet/arcade (github.com). TestUtilities.csproj in runtime needs to change as well.
Unfortunately, when testing .NETCoreApp our runtime specific test infra relies on the runtime being produced live. Changes are needed in the RunTests scripts and the msbuild tests files.
If you want to have those tests run in CI as well, changes to sendtohelix projects would be required as they have the same "Current" expectations hardcoded.

FWIW there are currently two severe issues affecting running tests locally in runtime:
Local running of libraries tests is broken · Issue #95762 · dotnet/runtime (github.com)
dotnet test is not running tests for .NETFramework targets · Issue #94183 · dotnet/runtime (github.com)

Copy link
Member

Choose a reason for hiding this comment

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

A workaround might be to have a separate test project that uses the net8.0 build of this assembly src project, but it would be running on latest runtime since that's what our tests do. The work to run on older frameworks is substantial as @ViktorHofer points out.

Copy link
Member

Choose a reason for hiding this comment

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

A workaround

Thanks, I'll try.

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 6 to 9
<PropertyGroup>
<!-- Workaround an OOM in LLVM -->
<WasmDedup>false</WasmDedup>
</PropertyGroup>
Copy link
Member Author

Choose a reason for hiding this comment

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

@vargaz, could I get sign-off on the workaround here. Same one as you recommended on the other issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks ok to me, still not sure why that test suite is affected though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure either. But it's not reproing elsewhere and is otherwise blocking the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vargaz, looks like it's getting a slightly different error now and still hitting an OOM later:

C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Failed to compile C:\helix\work\workitem\e\wasm_build\obj\wasm\for-build\System.Private.CoreLib.dll.bc -> C:\helix\work\workitem\e\wasm_build\obj\wasm\for-build\System.Private.CoreLib.dll.o [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : C:\helix\work\workitem\e\publish>C:\Windows\System32\chcp.com 65001 1>nul  [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : C:\helix\work\workitem\e\publish>setlocal [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : C:\helix\work\workitem\e\publish>set errorlevel=dummy  [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : C:\helix\work\workitem\e\publish>set errorlevel=  [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : C:\helix\work\workitem\e\publish>C:\helix\work\correlation\build\\wasi-sdk\\bin\clang.exe "@C:\helix\work\correlation\build\microsoft.netcore.app.runtime.wasi-wasm\runtimes\wasi-wasm\native\src\wasi-default.rsp" "@C:\helix\work\workitem\e\wasm_build\obj\wasm\for-build\wasi-compile-bc.rsp" -c -o "C:\Users\ContainerAdministrator\AppData\Local\Temp\tmp4k2wwb.tmp" "C:\helix\work\workitem\e\wasm_build\obj\wasm\for-build\System.Private.CoreLib.dll.bc"  [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : clang version 16.0.0 [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Target: wasm32-unknown-wasi [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Thread model: posix [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : InstalledDir: C:/helix/work/correlation/build//wasi-sdk//bin [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error :  (in-process) [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error :  "C:/helix/work/correlation/build/wasi-sdk/bin/clang.exe" -cc1 -triple wasm32-unknown-wasi -emit-obj -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name System.Private.CoreLib.dll.bc -mrelocation-model static -mframe-pointer=none -ffp-contract=on -fno-rounding-math -mconstructor-aliases -target-cpu generic -fvisibility=hidden -mllvm -treat-scalable-fixed-error-as-warning -debug-info-kind=constructor -dwarf-version=4 -debugger-tuning=gdb -v -fcoverage-compilation-dir=C:/helix/work/workitem/e/publish -resource-dir C:/helix/work/correlation/build/wasi-sdk/lib/clang/16 -Oz -fdebug-compilation-dir=C:/helix/work/workitem/e/publish -ferror-limit 19 -fgnuc-version=4.2.1 -vectorize-slp -o "C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\tmp4k2wwb.tmp" -x ir "C:\\helix\\work\\workitem\\e\\wasm_build\\obj\\wasm\\for-build\\System.Private.CoreLib.dll.bc" [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : clang -cc1 version 16.0.0 based upon LLVM 16.0.0 default target wasm32-wasi [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : LLVM ERROR: out of memory [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Allocation failed [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script. [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Stack dump: [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0.	Program arguments: C:\\helix\\work\\correlation\\build\\\\wasi-sdk\\\\bin\\clang.exe @C:\\helix\\work\\correlation\\build\\microsoft.netcore.app.runtime.wasi-wasm\\runtimes\\wasi-wasm\\native\\src\\wasi-default.rsp @C:\\helix\\work\\workitem\\e\\wasm_build\\obj\\wasm\\for-build\\wasi-compile-bc.rsp -c -o C:\\Users\\ContainerAdministrator\\AppData\\Local\\Temp\\tmp4k2wwb.tmp C:\\helix\\work\\workitem\\e\\wasm_build\\obj\\wasm\\for-build\\System.Private.CoreLib.dll.bc [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 1.	Code generation [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 2.	Running pass 'Function Pass Manager' on module 'C:\helix\work\workitem\e\wasm_build\obj\wasm\for-build\System.Private.CoreLib.dll.bc'. [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 3.	Running pass 'WebAssembly Explicit Locals' on function '@corlib_System_Buffers_IndexOfAnyAsciiSearcher_IndexOfAnyVectorized_System_Buffers_IndexOfAnyAsciiSearcher_DontNegate_System_Buffers_IndexOfAnyAsciiSearcher_Default_int16__int_System_Buffers_IndexOfAnyAsciiSearcher_AsciiState_' [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Exception Code: 0xC000001D [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x003CFB73 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x71B0C902 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x003B23A1 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x003B44C0 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x00C72CA9 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x00C72F30 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x00C72E9A <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x00C72E2C <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x0057829A <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x0281BFA0 <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : 0x02714C1C <unknown module> [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : clang: error: clang frontend command failed due to signal (use -v to see invocation) [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : clang version 16.0.0 [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Target: wasm32-unknown-wasi [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : Thread model: posix [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : InstalledDir: C:/helix/work/correlation/build//wasi-sdk//bin [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]
C:\helix\work\correlation\build\wasm-shared\WasmApp.Common.targets(819,5): error : clang: note: diagnostic msg: Error generating preprocessed source(s) - no preprocessable inputs. [took 152.83s] [C:\helix\work\workitem\e\publish\ProxyProjectForAOTOnHelix.proj]

@vargaz
Copy link
Contributor

vargaz commented Jan 10, 2024

/azp run runtime-wasm

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vargaz
Copy link
Contributor

vargaz commented Jan 11, 2024

One difference between the linked corlib with this PR is now Vector512 is not linked away, which causes a bunch more code to be AOTed. It looks like the problem is actually the methods added to AssertExtensions, those methods are not linked away for some reason, keeping Vector512 alive as well.

@tannergooding
Copy link
Member Author

It looks like the problem is actually the methods added to AssertExtensions, those methods are not linked away for some reason, keeping Vector512 alive as well.

I've relocated those to only be in System.Runtime.Intrinsics.Tests for the time being, which should hopefully help avoid the issue.

@tannergooding tannergooding merged commit a3d5261 into dotnet:main Jan 11, 2024
178 checks passed
@tannergooding tannergooding deleted the fix-93513 branch January 11, 2024 21:49
tmds pushed a commit to tmds/runtime that referenced this pull request Jan 23, 2024
…tnet#96455)

* Adding vectorized implementations of Log2 to Vector64/128/256/512

* Accelerate TensorPrimitives.Log2<double>()

* Undo a sln file change

* Use the built-in Log2 implementation from Vector64/128/256/512 on .NET 9+

* Ensure the helpers are checking the correct types

* Relocate the Vector64/128/256/512 assert extensions to avoid Mono LLVM trimming issues

* Keep xunit happy
@github-actions github-actions bot locked and limited conversation to collaborators Feb 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants