Skip to content

JIT: Fix double handling of call operands when importing intrinsic #96557

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.

Sign up for GitHub

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

Merged
jakobbotsch merged 2 commits into from
Jan 5, 2024
Merged

JIT: Fix double handling of call operands when importing intrinsic #96557

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e0dc28
Add test
jakobbotsch Jan 5, 2024
9580bed
JIT: Fix double handling of call operands when importing intrinsic
jakobbotsch Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 4 additions & 12 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,11 +1303,10 @@ var_types Compiler::impImportCall(OPCODE opcode,
eeGetCallSiteSig(pResolvedToken->token, pResolvedToken->tokenScope, pResolvedToken->tokenContext, sig);
}

typeInfo tiRetVal = verMakeTypeInfo(sig->retType, sig->retTypeClass);

if (call->IsCall())
// Sometimes "call" is not a GT_CALL (if we imported an intrinsic that didn't turn into a call)
if (!bIntrinsicImported)
Copy link
Member

@tannergooding tannergooding Jan 5, 2024

Choose a reason for hiding this comment

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

Is this something that needs backported?

Looks like it's been a potential issue for a long time, although it's unclear to me why we're only just seeing this "now" given how widely used intrinsics have been, including in potentially tail recursive positions, and we've many that can be no-ops or just return one of the input operands (such as op_UnaryPlus, AsInt32, etc).

-- Notably looks like there are some asserts in this block checking that !bIntrinsicImported that can potentially be removed.

Copy link
Member

@AndyAyersMS AndyAyersMS Jan 5, 2024

Choose a reason for hiding this comment

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

Seems likely this double-processing ended up with the same result, so didn't actually cause a bug.

Copy link
Member Author

@jakobbotsch jakobbotsch Jan 5, 2024

Choose a reason for hiding this comment

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

I don't think it needs to be backported, I think it's very hard for the code that runs twice here to cause problems in release builds. Also we need a customer report for backporting.

Looks like it's been a potential issue for a long time, although it's unclear to me why we're only just seeing this "now" given how widely used intrinsics have been, including in potentially tail recursive positions, and we've many that can be no-ops or just return one of the input operands (such as op_UnaryPlus, AsInt32, etc).

Presumably people do not write code that involves noop intrinsics like op_UnaryPlus very often, and even more rarely that also satisfies the other conditions to hit the bug.

-- Notably looks like there are some asserts in this block checking that !bIntrinsicImported that can potentially be removed.

Let me address that as part of a future PR to rerunning CI.

{
// Sometimes "call" is not a GT_CALL (if we imported an intrinsic that didn't turn into a call)
assert(call->IsCall());

GenTreeCall* origCall = call->AsCall();

Expand Down Expand Up @@ -1423,10 +1422,7 @@ var_types Compiler::impImportCall(OPCODE opcode,
impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("non-inline candidate call"));
}
}
}

if (!bIntrinsicImported)
{
//-------------------------------------------------------------------------
//
/* If the call is of a small type and the callee is managed, the callee will normalize the result
Expand All @@ -1441,14 +1437,10 @@ var_types Compiler::impImportCall(OPCODE opcode,
}
}

typeInfo tiRetVal = verMakeTypeInfo(sig->retType, sig->retTypeClass);
impPushOnStack(call, tiRetVal);
}

// VSD functions get a new call target each time we getCallInfo, so clear the cache.
// Also, the call info cache for CALLI instructions is largely incomplete, so clear it out.
// if ( (opcode == CEE_CALLI) || (callInfoCache.fetchCallInfo().kind == CORINFO_VIRTUALCALL_STUB))
// callInfoCache.uncacheCallInfo();

return callRetTyp;
}
#ifdef _PREFAST_
Expand Down
19 changes: 19 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_96461/Runtime_96461.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using Xunit;

public class Runtime_96461
{
[Fact]
public static int TestEntryPoint()
{
Vector128<int> result = Unsafe.BitCast<Vector128<int>, Vector128<int>>(V());
return result[0];
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static Vector128<int> V() => Vector128.Create(100);
}
10 changes: 10 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_96461/Runtime_96461.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
<DebugType>None</DebugType>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>