JIT: contained memory safety analysis fixes and improvements #64843
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4723,7 +4723,7 @@ void Lowering::ContainCheckDivOrMod(GenTreeOp* node) | |
| #endif | ||
|
|
||
| // divisor can be an r/m, but the memory indirection must be of the same size as the divide | ||
| if (IsContainableMemoryOp(divisor) && (divisor->TypeGet() == node->TypeGet()) && IsSafeToContainMem(node, divisor)) | ||
| { | ||
| MakeSrcContained(node, divisor); | ||
| } | ||
|
|
@@ -4853,7 +4853,7 @@ void Lowering::ContainCheckCast(GenTreeCast* node) | |
| // U8 -> R8 conversion requires that the operand be in a register. | ||
| if (srcType != TYP_ULONG) | ||
| { | ||
| if ((IsContainableMemoryOp(castOp) && IsSafeToContainMem(node, castOp)) || castOp->IsCnsNonZeroFltOrDbl()) | ||
| { | ||
| MakeSrcContained(node, castOp); | ||
| } | ||
|
|
@@ -4949,7 +4949,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp) | |
| // we can treat the MemoryOp as contained. | ||
| if (op1Type == op2Type) | ||
| { | ||
| if (IsContainableMemoryOp(op1) && IsSafeToContainMem(cmp, op1)) | ||
|
Member
There was a problem hiding this comment. There looked to be a couple outer places in lowerxarch that do
We also have https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/lowerxarch.cpp#L5072-L5075, where its not clear if it doing
Member
Author
There was a problem hiding this comment. Thanks, added checks. I'll probably leave that last bit as is, |
||
| { | ||
| MakeSrcContained(cmp, op1); | ||
| } | ||
|
|
@@ -5250,7 +5250,7 @@ void Lowering::ContainCheckBoundsChk(GenTreeBoundsChk* node) | |
|
|
||
| if (node->GetIndex()->TypeGet() == node->GetArrayLength()->TypeGet()) | ||
| { | ||
| if (IsContainableMemoryOp(other) && IsSafeToContainMem(node, other)) | ||
| { | ||
| MakeSrcContained(node, other); | ||
| } | ||
|
|
@@ -5279,7 +5279,7 @@ void Lowering::ContainCheckIntrinsic(GenTreeOp* node) | |
| { | ||
| GenTree* op1 = node->gtGetOp1(); | ||
|
|
||
| if ((IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1)) || op1->IsCnsNonZeroFltOrDbl()) | ||
| { | ||
| MakeSrcContained(node, op1); | ||
| } | ||
|
|
@@ -5365,6 +5365,7 @@ void Lowering::ContainCheckSIMD(GenTreeSIMD* simdNode) | |
| // [In/Out] pNode - The node to check and potentially replace with the containable node | ||
| // [Out] supportsRegOptional - On return, this will be true if 'containingNode' supports regOptional operands | ||
| // otherwise, false. | ||
| // [In] transparentParentNode - optional "transparent" intrinsic parent like CreateScalarUnsafe | ||
| // | ||
| // Return Value: | ||
| // true if 'node' is a containable by containingNode; otherwise, false. | ||
|
|
@@ -5377,7 +5378,8 @@ void Lowering::ContainCheckSIMD(GenTreeSIMD* simdNode) | |
| // | ||
| bool Lowering::TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode, | ||
| GenTree** pNode, | ||
| bool* supportsRegOptional, | ||
| GenTreeHWIntrinsic* transparentParentNode) | ||
| { | ||
| assert(containingNode != nullptr); | ||
| assert((pNode != nullptr) && (*pNode != nullptr)); | ||
|
|
@@ -5800,7 +5802,32 @@ bool Lowering::TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode | |
|
|
||
| if (!node->OperIsHWIntrinsic()) | ||
| { | ||
| bool canBeContained = false; | ||
|
|
||
| if (supportsGeneralLoads) | ||
| { | ||
| if (IsContainableMemoryOp(node)) | ||
| { | ||
| // Code motion safety checks | ||
| // | ||
| if (transparentParentNode != nullptr) | ||
| { | ||
| canBeContained = IsSafeToContainMem(containingNode, transparentParentNode, node); | ||
|
Member
There was a problem hiding this comment. We don't need this for the
Member
Author
There was a problem hiding this comment. I think so, yes. But not 100% sure. By my understanding, for unary containing nodes, the safety check should immediately return safe, as the child's
Member
Author
There was a problem hiding this comment. I'm going to add a quick early out to
Contributor
There was a problem hiding this comment.
Do you mean that for unary operators t1 = IND(addr)
STOREIND(addr, ...) // Updates [addr] to t2
UNARY_USER(t1) // Should observe t1, not t2.Is valid LIR.
Member
Author
There was a problem hiding this comment. Do you know of any way we'd create such a pattern? It would be nice to have more examples that fail if we mess this up. At any rate, best not to be too clever here. With the added safety calls and follow-up check in
Contributor
There was a problem hiding this comment.
Yep: private static uint Problem(uint a, uint* b)
{
uint zero = 0;
return *b + Bmi2.MultiplyNoFlags(a, a, b) * zero;
}N003 (???,???) [000015] ------------ IL_OFFSET void INLRT @ 0x002[E-] REG NA
N005 ( 1, 1) [000003] -----------Z t3 = LCL_VAR int V01 arg1 u:1 edx REG edx $81
/--* t3 int
N007 ( 3, 2) [000004] *--XG------- t4 = * IND int REG eax <l:$1c1, c:$1c0>
N009 ( 1, 1) [000005] ------------ t5 = LCL_VAR int V00 arg0 u:1 ecx REG ecx $80
N011 ( 1, 1) [000006] ------------ t6 = LCL_VAR int V00 arg0 u:1 ecx (last use) REG ecx $80
N013 ( 1, 1) [000007] -----------z t7 = LCL_VAR int V01 arg1 u:1 esi (last use) REG esi $81
/--* t5 int
+--* t6 int
+--* t7 int
N015 ( 4, 4) [000008] ---XG------- t8 = * HWINTRINSIC int MultiplyNoFlags REG edx $83
/--* t4 int
N017 ( 10, 9) [000012] ---XG------- * RETURN int REG NA $181
|
||
| } | ||
| else | ||
| { | ||
| canBeContained = IsSafeToContainMem(containingNode, node); | ||
| } | ||
| } | ||
| else if (node->IsCnsNonZeroFltOrDbl()) | ||
| { | ||
| // Always safe. | ||
| // | ||
| canBeContained = true; | ||
| } | ||
| } | ||
|
|
||
| return canBeContained; | ||
| } | ||
|
|
||
| // TODO-XArch: Update this to be table driven, if possible. | ||
|
|
@@ -5821,7 +5848,7 @@ bool Lowering::TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode | |
| GenTree* op1 = hwintrinsic->Op(1); | ||
| bool op1SupportsRegOptional = false; | ||
|
|
||
| if (!TryGetContainableHWIntrinsicOp(containingNode, &op1, &op1SupportsRegOptional, hwintrinsic)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
@@ -6287,7 +6314,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node) | |
| MakeSrcContained(node, op2); | ||
| } | ||
|
|
||
| if (IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1)) | ||
| { | ||
| MakeSrcContained(node, op1); | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| // Generated by Fuzzlyn v1.5 on 2022-02-04 12:51:20 | ||
| // Run on X86 Windows | ||
| // Seed: 5821979800164656837 | ||
| // Reduced from 60.0 KiB to 0.3 KiB in 00:01:44 | ||
| // Debug: Outputs 0 | ||
| // Release: Outputs -1 | ||
| public class Program | ||
| { | ||
| public static IRT s_rt; | ||
| public static long s_4; | ||
| public static int Main() | ||
| { | ||
| s_rt = new C(); | ||
| int vr3 = (int)(s_4 & ~M7()); | ||
| s_rt.WriteLine(vr3); | ||
| return vr3 + 100; | ||
| } | ||
|
|
||
| public static short M7() | ||
| { | ||
| ref long var1 = ref s_4; | ||
| var1 = 9223372036854775807L; | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public interface IRT | ||
| { | ||
| void WriteLine<T>(T value); | ||
| } | ||
|
|
||
| public class C : IRT | ||
| { | ||
| public void WriteLine<T>(T value) | ||
| { | ||
| System.Console.WriteLine(value); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <OutputType>Exe</OutputType> | ||
| <DebugType /> | ||
| <Optimize>True</Optimize> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Include="$(MSBuildProjectName).cs" /> | ||
| </ItemGroup> | ||
| </Project> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| // Generated by Fuzzlyn v1.5 on 2022-02-04 13:20:27 | ||
| // Run on X64 Windows | ||
| // Seed: 7242107031351932946 | ||
| // Reduced from 319.7 KiB to 0.7 KiB in 00:09:00 | ||
| // Debug: | ||
| // Release: Outputs 0 | ||
| public class C0 | ||
| { | ||
| public byte F1; | ||
| } | ||
|
|
||
| public struct S0 | ||
| { | ||
| public C0 F0; | ||
| public S0(C0 f0): this() | ||
| { | ||
| F0 = f0; | ||
| } | ||
| } | ||
|
|
||
| public class C1 | ||
| { | ||
| public long F0; | ||
| public C1(long f0) | ||
| { | ||
| F0 = f0; | ||
| } | ||
| } | ||
|
|
||
| public class Program | ||
| { | ||
| public static S0 s_25 = new S0(new C0()); | ||
| public static C1[] s_28; | ||
| public static int Main() | ||
| { | ||
| int result = 100; | ||
| s_28 = new C1[]{new C1(0)}; | ||
| var vr3 = new C1(-1); | ||
| if (vr3.F0 >= (M35(vr3) & 0)) | ||
| { | ||
| System.Console.WriteLine(vr3.F0); | ||
| result = -1; | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public static byte M35(C1 argThis) | ||
| { | ||
| argThis.F0 = s_28[0].F0; | ||
| return s_25.F0.F1; | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like there's a few argument names here that need to be updated,
ancestorNodeandgrandParentNode=>grandparentNode?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- will fix this subsequently so I don't retrigger all the tests.