Skip to content

Commit

Permalink
JIT: Fix generating unrepresentable nullchecks on ARM64 (#71687)
Browse files Browse the repository at this point in the history
The logic in lowering could in some struct and primitive type cases
generate an unrepresentable NULLCHECK node for ARM64/LA64 when there
was a contained address mode. We now transform unused indirections and
create address modes in the opposite order on non-xarch to avoid these
unrepresentable nullchecks.

Fix #71684
  • Loading branch information
jakobbotsch authored Jul 6, 2022
1 parent a965218 commit 9534947
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 3 deletions.
11 changes: 8 additions & 3 deletions src/coreclr/jit/compiler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9992,16 +9992,21 @@ bool Compiler::lvaIsOSRLocal(unsigned varNum)
//
var_types Compiler::gtTypeForNullCheck(GenTree* tree)
{
if (varTypeIsArithmetic(tree))
static const var_types s_typesBySize[] = {TYP_UNDEF, TYP_BYTE, TYP_SHORT, TYP_UNDEF, TYP_INT,
TYP_UNDEF, TYP_UNDEF, TYP_UNDEF, TYP_LONG};

if (!varTypeIsStruct(tree))
{
#if defined(TARGET_XARCH)
// Just an optimization for XARCH - smaller mov
if (varTypeIsLong(tree))
if (genTypeSize(tree) == 8)
{
return TYP_INT;
}
#endif
return tree->TypeGet();

assert((genTypeSize(tree) < ARRAY_SIZE(s_typesBySize)) && (s_typesBySize[genTypeSize(tree)] != TYP_UNDEF));
return s_typesBySize[genTypeSize(tree)];
}
// for the rest: probe a single byte to avoid potential AVEs
return TYP_BYTE;
Expand Down
13 changes: 13 additions & 0 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7149,6 +7149,17 @@ void Lowering::LowerIndir(GenTreeIndir* ind)
// they only appear as the source of a block copy operation or a return node.
if (!ind->TypeIs(TYP_STRUCT) || ind->IsUnusedValue())
{
#ifndef TARGET_XARCH
// On non-xarch, whether or not we can contain an address mode will depend on the access width
// which may be changed when transforming an unused indir, so do that first.
// On xarch, it is the opposite: we transform to indir/nullcheck based on whether we contained the
// address mode, so in that case we must do this transformation last.
if (ind->OperIs(GT_NULLCHECK) || ind->IsUnusedValue())
{
TransformUnusedIndirection(ind, comp, m_block);
}
#endif

// TODO-Cleanup: We're passing isContainable = true but ContainCheckIndir rejects
// address containment in some cases so we end up creating trivial (reg + offfset)
// or (reg + reg) LEAs that are not necessary.
Expand All @@ -7165,10 +7176,12 @@ void Lowering::LowerIndir(GenTreeIndir* ind)
TryCreateAddrMode(ind->Addr(), isContainable, ind);
ContainCheckIndir(ind);

#ifdef TARGET_XARCH
if (ind->OperIs(GT_NULLCHECK) || ind->IsUnusedValue())
{
TransformUnusedIndirection(ind, comp, m_block);
}
#endif
}
else
{
Expand Down
47 changes: 47 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_71687/Runtime_71687.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;

class Runtime_71687
{
[MethodImpl(MethodImplOptions.NoInlining)]
private static void Test<T>(ref T first, int i)
{
Consume(Unsafe.Add(ref first, i));
}

// Must be inlined so we end up with null check above
private static void Consume<T>(T value) { }

private static int Main()
{
Test(ref (new byte[10])[0], 5);
Test(ref (new sbyte[10])[0], 5);
Test(ref (new ushort[10])[0], 5);
Test(ref (new short[10])[0], 5);
Test(ref (new uint[10])[0], 5);
Test(ref (new int[10])[0], 5);
Test(ref (new ulong[10])[0], 5);
Test(ref (new long[10])[0], 5);
Test(ref (new float[10])[0], 5);
Test(ref (new double[10])[0], 5);
Test(ref (new object[10])[0], 5);
Test(ref (new string[10])[0], 5);
Test(ref (new Vector<float>[10])[0], 5);
Test(ref (new Vector128<float>[10])[0], 5);
Test(ref (new Vector256<float>[10])[0], 5);
Test(ref (new Struct1[10])[0], 5);
Test(ref (new Struct2[10])[0], 5);
Test(ref (new Struct4[10])[0], 5);
Test(ref (new Struct8[10])[0], 5);
return 100;
}

private struct Struct1 { public byte Field; }
private struct Struct2 { public short Field; }
private struct Struct4 { public int Field; }
private struct Struct8 { public long Field; }
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>

0 comments on commit 9534947

Please sign in to comment.