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.

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

Jump to bottom

Fix passing too small args for PUTARG_STK on macOS arm64 ABI #68108

Merged
merged 2 commits into from
Apr 16, 2022
Merged

Fix passing too small args for PUTARG_STK on macOS arm64 ABI #68108

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
dfbae00
Fix passing too small args for PUTARG_STK on macOS arm64 ABI
jakobbotsch Apr 16, 2022
d682d68
Fix test namespace
jakobbotsch Apr 16, 2022
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
32 changes: 13 additions & 19 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -738,17 +738,6 @@ void CodeGen::genIntrinsic(GenTree* treeNode)
void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
{
assert(treeNode->OperIs(GT_PUTARG_STK));
GenTree* source = treeNode->gtOp1;
var_types targetType;

if (!compMacOsArm64Abi())
{
targetType = genActualType(source->TypeGet());
}
else
{
targetType = source->TypeGet();
}
emitter* emit = GetEmitter();

// This is the varNum for our store operations,
Expand Down Expand Up @@ -792,11 +781,13 @@ void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
argOffsetMax = compiler->lvaOutgoingArgSpaceSize;
}

bool isStruct = (targetType == TYP_STRUCT) || (source->OperGet() == GT_FIELD_LIST);
GenTree* source = treeNode->gtGetOp1();

bool isStruct = source->TypeIs(TYP_STRUCT) || (source->OperGet() == GT_FIELD_LIST);

if (!isStruct) // a normal non-Struct argument
{
if (varTypeIsSIMD(targetType))
if (varTypeIsSIMD(source->TypeGet()))
{
assert(!source->isContained());

Expand All @@ -815,32 +806,35 @@ void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
else
#endif // TARGET_ARM64
{
emitAttr storeAttr = emitTypeSize(targetType);
emitAttr storeAttr = emitTypeSize(source->TypeGet());
emit->emitIns_S_R(INS_str, storeAttr, srcReg, varNumOut, argOffsetOut);
argOffsetOut += EA_SIZE_IN_BYTES(storeAttr);
}
assert(argOffsetOut <= argOffsetMax); // We can't write beyond the outgoing arg area
return;
}

var_types slotType = genActualType(source);
Copy link
Contributor

Choose a reason for hiding this comment

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

do I understand this change correctly that now we have such slot types:
1byte - TYP_BYTE
2bytes - TYP_SHORT
4bytes - TYP_LONG (was TYP_INT before the change).
8bytes - TYP_LONG
?
so if we pass two TYP_INT on the stack, in one 8byte stack slot, don't we override anything?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not totally sure I understand. genActualType(TYP_INT) returns TYP_INT, not TYP_LONG.

The difference before was that we used the real type of the operand to decide on how much to store on the stack. That's not right, a small-typed operand still produces a value of type genActualType(operand) and it is the PUTARG_STK node that decides how much should be put on the stack. So we had a problem for e.g. a TYP_SHORT indirection feeding into a TYP_INT parameter.

A TYP_INT indirection feeding into a 4 byte parameter will still only store 4 bytes on the stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks!

if (compMacOsArm64Abi())
Copy link
Member

Choose a reason for hiding this comment

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

@jakobbotsch, just for my own education; is it specific to mac ABI? i.e. if we apply the same classification to linux and windows arm64, it will break putarg on those targets?

This TODO is probably related:

runtime/src/coreclr/jit/morph.cpp

Lines 736 to 738 in d682d68

// TODO-Cleanup: structs are aligned to 8 bytes on arm64 apple, so it would work, but pass the precise size.
*nextSlotNum += numSlots;
m_nextStackByteOffset += numSlots * TARGET_POINTER_SIZE;

Copy link
Member Author

Choose a reason for hiding this comment

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

macOS arm64 ABI is different in that multiple small arguments can be packed into the same stack slot. It means we have different behavior in some places in the JIT, and that different behavior was wrong here.
The behavior for the other ABIs did not have this bug.

{
// Small typed args do not get their own full stack slots, so make
// sure we do not overwrite adjacent arguments.
switch (treeNode->GetStackByteSize())
{
case 1:
targetType = TYP_BYTE;
slotType = TYP_BYTE;
break;
case 2:
targetType = TYP_SHORT;
slotType = TYP_SHORT;
break;
default:
assert(treeNode->GetStackByteSize() >= 4);
break;
}
}

instruction storeIns = ins_Store(targetType);
emitAttr storeAttr = emitTypeSize(targetType);
instruction storeIns = ins_Store(slotType);
emitAttr storeAttr = emitTypeSize(slotType);

// If it is contained then source must be the integer constant zero
if (source->isContained())
Expand All @@ -860,7 +854,7 @@ void CodeGen::genPutArgStk(GenTreePutArgStk* treeNode)
genConsumeReg(source);
emit->emitIns_S_R(storeIns, storeAttr, source->GetRegNum(), varNumOut, argOffsetOut);
#ifdef TARGET_ARM
if (targetType == TYP_LONG)
if (source->TypeIs(TYP_LONG))
{
// This case currently only occurs for double types that are passed as TYP_LONG;
// actual long types would have been decomposed by now.
Expand Down
29 changes: 29 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_66720/Runtime_66720.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// 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;

public class Runtime_66720
{
public static int Main()
{
return Test(0);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Test(in short zero)
{
// Fill arg stack slot with all ones
LastArg(0, 0, 0, 0, 0, 0, 0, 0, -1);
// Bug was that the last arg passed here would write only 16 bits
// instead of 32 bits
int last = LastArg(0, 0, 0, 0, 0, 0, 0, 0, zero);
return last == 0 ? 100 : -1;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static int LastArg(int a, int b, int c, int d, int e, int f, int g, int h, int i)
{
return i;
}
}
9 changes: 9 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_66720/Runtime_66720.csproj
View file
Open in desktop
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>