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

Use TensorPrimitives.ElementWiseSelect instead of ConditionalSelect in more places #97282

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

stephentoub
Copy link
Member

Also special-case nint/nuint in ElementWiseSelect.

@ghost
Copy link

ghost commented Jan 22, 2024

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

Issue Details

Also special-case nint/nuint in ElementWiseSelect.

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Numerics

Milestone: -

@stephentoub
Copy link
Member Author

@BrzVlad, mono interpreter legs are crashing:

/root/helix/work/workitem/e /root/helix/work/workitem/e
  Discovering: System.Numerics.Tensors.Net8.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Numerics.Tensors.Net8.Tests (found 4087 test cases)
  Starting:    System.Numerics.Tensors.Net8.Tests (parallel test collections = on, max threads = 2)
./RunTests.sh: line 180:    22 Killed                  "$RUNTIME_PATH/dotnet" exec --runtimeconfig System.Numerics.Tensors.Net8.Tests.runtimeconfig.json --depsfile System.Numerics.Tensors.Net8.Tests.deps.json xunit.console.dll System.Numerics.Tensors.Net8.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing $RSP_FILE
/root/helix/work/workitem/e

Do you want to take a look?

@BrzVlad
Copy link
Member

BrzVlad commented Jan 22, 2024

For me the issue is not interp only, it happens also on mono jit. Not sure how the tests passed on this PR. It seems lately that System.Numerics.Tensors regressed in execution time and mem usage, on mono. This PR seemed to put the nail in the coffin since the test suite is never ending, reaching 10+ GB of memusage. Seems like the problem comes from compilling methods with a lot of static conditionals. CoreCLR just casually executes this suite in 30s with a few hundreds MB of mem usage.

I'm wondering how CoreCLR handles cases like:

if (cond1)
   Method1 ()
else if (cond2)
   Method2 ()
else ....

Where cond1, cond2 ... are easily known at compile time (generic argument type comparison, IsSupported etc.). The problem in mono is that optimizations are run after the full initial IL is processed. This means that even if cond1 is false, if Method1 is aggressive inlining and massive, it will still get inlined, so we end up with compilation taking a long time with a lot of dead code to be removed. Is RyuJit doing something to prevent this redundant inlining ? This pattern becomes ever more common, and I think might also impact startup times on mono.

@stephentoub
Copy link
Member Author

This pattern becomes ever more common

Yes, it's being used in many more places now, especially with intrinsics/vectorization, special-casing for different types, using generic specialization to drive different behaviors based on call site, etc.

Is RyuJit doing something to prevent this redundant inlining ?

cc: @EgorBo, @tannergooding

@EgorBo
Copy link
Member

EgorBo commented Jan 22, 2024

Is RyuJit doing something to prevent this redundant inlining ?

Yes, RyuJIT eliminates simple branches like that before it starts inlining, e.g.

if (Sse2.IsSupported)
{
    call();
}

it never tries to import/inline that call basically

@tannergooding
Copy link
Member

There is already a PR up for Mono that is attempting to address some of the memory usage issues it is seeing: #97096

@MichalPetryka
Copy link
Contributor

Is RyuJit doing something to prevent this redundant inlining ?

Also it's worth noting here that RyuJIT specializes all value types, so it can fold type checks on those too.

@EgorBo
Copy link
Member

EgorBo commented Jan 22, 2024

There is already a PR up for Mono that is attempting to address some of the memory usage issues it is seeing: #97096

I believe that one doesn't fix the root cause - it still inlines stuff in all blocks, it just removes them before it starts emitting LLVM IR.

@stephentoub
Copy link
Member Author

To avoid unnecessarily exacerbating the problem, I trimmed this back to just the condensing of the ElementWiseSelect if checks.

But we will need to address the underlying mono issues around this. This kind of code is only going to increase.

@stephentoub stephentoub merged commit d523506 into dotnet:main Jan 24, 2024
108 of 111 checks passed
@stephentoub stephentoub deleted the elementwiseselect branch January 24, 2024 02:05
@github-actions github-actions bot locked and limited conversation to collaborators Feb 23, 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