Skip to content

JIT: Copy loop memory dependence recursively on hoisting #116068

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 1 commit into from
May 28, 2025
Merged

JIT: Copy loop memory dependence recursively on hoisting #116068

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 21 additions & 31 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3674,30 +3674,7 @@ void Compiler::optPerformHoistExpr(GenTree* origExpr, BasicBlock* exprBb, FlowGr

preheader->CopyFlags(exprBb, BBF_COPY_PROPAGATE);

Statement* hoistStmt = gtNewStmt(hoist);

// Simply append the statement at the end of the preHead's list.
Statement* firstStmt = preheader->firstStmt();
if (firstStmt != nullptr)
{
/* append after last statement */

Statement* lastStmt = preheader->lastStmt();
assert(lastStmt->GetNextStmt() == nullptr);

lastStmt->SetNextStmt(hoistStmt);
hoistStmt->SetPrevStmt(lastStmt);
firstStmt->SetPrevStmt(hoistStmt);
}
else
{
/* Empty pre-header - store the single statement in the block */

preheader->bbStmtList = hoistStmt;
hoistStmt->SetPrevStmt(hoistStmt);
}

hoistStmt->SetNextStmt(nullptr);
fgInsertStmtAtEnd(preheader, fgNewStmtFromTree(hoist));

#ifdef DEBUG
if (verbose)
Expand All @@ -3708,12 +3685,6 @@ void Compiler::optPerformHoistExpr(GenTree* origExpr, BasicBlock* exprBb, FlowGr
}
#endif

if (fgNodeThreading == NodeThreading::AllTrees)
{
gtSetStmtInfo(hoistStmt);
fgSetStmtSeq(hoistStmt);
}

#ifdef DEBUG
if (m_nodeTestData != nullptr)
{
Expand Down Expand Up @@ -4304,22 +4275,41 @@ void Compiler::optRecordLoopMemoryDependence(GenTree* tree, BasicBlock* block, V
}

//------------------------------------------------------------------------
// optCopyLoopMemoryDependence: record that tree's loop memory dependence
// optCopyLoopMemoryDependence: Recursively record that tree's loop memory dependence
// is the same as some other tree.
//
// Arguments:
// fromTree -- tree to copy dependence from
// toTree -- tree in question
//
// Remarks:
// This requires 'toTree' to be in its own statement
//
Comment on lines +4286 to +4287
Copy link

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The remark about 'toTree' needing to be in its own statement is unclear. It would help to clarify the precondition (e.g., both trees must be part of the same statement sequence) or remove if not needed.

Suggested change
// This requires 'toTree' to be in its own statement
//
// 'toTree' must be part of a separate statement sequence to ensure proper
// traversal and mapping in the loop memory dependence analysis.

Copilot uses AI. Check for mistakes.
void Compiler::optCopyLoopMemoryDependence(GenTree* fromTree, GenTree* toTree)
{
assert(fromTree->OperGet() == toTree->OperGet());

NodeToLoopMemoryBlockMap* const map = GetNodeToLoopMemoryBlockMap();
BasicBlock* mapBlock = nullptr;

if (map->Lookup(fromTree, &mapBlock))
{
map->Set(toTree, mapBlock);
}

GenTreeOperandIterator fromIterCur = fromTree->OperandsBegin();
GenTreeOperandIterator fromIterEnd = fromTree->OperandsEnd();
GenTreeOperandIterator toIterCur = toTree->OperandsBegin();
GenTreeOperandIterator toIterEnd = toTree->OperandsEnd();

while (fromIterCur != fromIterEnd)
{
optCopyLoopMemoryDependence(*fromIterCur, *toIterCur);
++fromIterCur;
++toIterCur;
}

assert(toIterCur == toIterEnd);
}

//------------------------------------------------------------------------
Expand Down
46 changes: 46 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_115109/Runtime_115109.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// 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 v2.8 on 2025-04-27 22:46:08
// Run on X86 Windows
// Seed: 2255917678885586044-vectort,vector128,vector256,x86aes,x86avx,x86avx2,x86avx512bw,x86avx512bwvl,x86avx512cd,x86avx512cdvl,x86avx512dq,x86avx512dqvl,x86avx512f,x86avx512fvl,x86bmi1,x86bmi2,x86fma,x86lzcnt,x86pclmulqdq,x86popcnt,x86sse,x86sse2,x86sse3,x86sse41,x86sse42,x86ssse3,x86x86base
// Reduced from 118.3 KiB to 1.0 KiB in 00:17:25
// Debug: Outputs <0, 0, 0, 0, 0, 0, 0, 0>
// Release: Outputs <1, 0, 0, 0, 0, 0, 0, 0>
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using Xunit;

public class Runtime_115109
{
static int s_5 = 101;
static Vector<int>[] s_10;
static byte s_21;

[Fact]
public static int TestEntryPoint()
{
s_10 = [Vector128.CreateScalar(0).AsVector()];
bool vr5 = 0 <= M10();
Copy link

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

[nitpick] The variable vr5 is assigned but never used; consider removing it or using the result directly in the return or an assertion to make its purpose clear.

Suggested change
bool vr5 = 0 <= M10();
Assert.True(0 <= M10(), "Expected M10() to return a value greater than or equal to 0.");

Copilot uses AI. Check for mistakes.
Copy link
Member Author

@jakobbotsch jakobbotsch May 28, 2025

Choose a reason for hiding this comment

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

Fair point, but I don't think it's worth rerunning CI.

return s_10[0][0];
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static byte M10()
{
byte lvar2 = 3;
do
{
s_5 = 100;
int lvar4 = 0;
do
{
s_10[0] = Vector128.CreateScalar(s_5).AsVector();
}
while (++lvar4 < 2);
}
while (--lvar2 > 1);
return s_21;
}
}
8 changes: 8 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_115109/Runtime_115109.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
Copy link

Copilot AI May 28, 2025

Choose a reason for hiding this comment

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

The project file is missing a <TargetFramework> entry, so it may fail to build. Please add something like <TargetFramework>net7.0</TargetFramework> under the PropertyGroup.

Suggested change
<Optimize>True</Optimize>
<Optimize>True</Optimize>
<TargetFramework>net7.0</TargetFramework>

Copilot uses AI. Check for mistakes.
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading