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

Loop cloning for Span #113575

Merged
merged 1 commit into from
Mar 20, 2025
+250 −11
Merged

Loop cloning for Span #113575

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
1 change: 1 addition & 0 deletions src/coreclr/jit/clrjit.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ Documentation for VS debugger format specifiers: https://learn.microsoft.com/vis
<Expand>
<ExpandedItem Condition="optType == LcOptInfo::OptType::LcJaggedArray">(LcJaggedArrayOptInfo*)this,nd</ExpandedItem>
<ExpandedItem Condition="optType == LcOptInfo::OptType::LcMdArray">(LcMdArrayOptInfo*)this,nd</ExpandedItem>
<ExpandedItem Condition="optType == LcOptInfo::OptType::LcSpan">(LcSpanOptInfo*)this,nd</ExpandedItem>
</Expand>
</Type>

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8185,6 +8185,7 @@ class Compiler

bool optIsStackLocalInvariant(FlowGraphNaturalLoop* loop, unsigned lclNum);
bool optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsNum, bool* topLevelIsFinal);
bool optExtractSpanIndex(GenTree* tree, SpanIndex* result);
bool optReconstructArrIndexHelp(GenTree* tree, ArrIndex* result, unsigned lhsNum, bool* topLevelIsFinal);
bool optReconstructArrIndex(GenTree* tree, ArrIndex* result);
bool optIdentifyLoopOptInfo(FlowGraphNaturalLoop* loop, LoopCloneContext* context);
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/importercalls.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3671,6 +3671,8 @@ GenTree* Compiler::impIntrinsic(CORINFO_CLASS_HANDLE clsHnd,
case NI_System_Span_get_Item:
case NI_System_ReadOnlySpan_get_Item:
{
optMethodFlags |= OMF_HAS_ARRAYREF;

// Have index, stack pointer-to Span<T> s on the stack. Expand to:
//
// For Span<T>
Expand Down
173 changes: 162 additions & 11 deletions src/coreclr/jit/loopcloning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,22 @@ void ArrIndex::PrintBoundsCheckNodes(unsigned dim /* = -1 */)
}
}

//--------------------------------------------------------------------------------------------------
// Print: debug print an SpanIndex struct in form: `V01[V02]`.
//
void SpanIndex::Print()
{
printf("V%02d[V%02d]", lenLcl, indLcl);
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit odd, as it indicates that lenLcl is an "array", but it's actually just the length of the array (in the span). Maybe use something like Span<V%02d>[V%02d] instead? Basically, something to make it obvious that lenLcl is not an array.

Copy link
Member Author

@EgorBo EgorBo Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

yep, copy-paste from ArrIndex 🙂 I'll fix the comment in a follow up (I think I found a few missing opportunities for LC) to avoid spinning the CI again

}

//--------------------------------------------------------------------------------------------------
// PrintBoundsCheckNode: - debug print an SpanIndex struct bounds check node tree id
//
void SpanIndex::PrintBoundsCheckNode()
{
Compiler::printTreeID(bndsChk);
}

#endif // DEBUG

//--------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -115,6 +131,20 @@ GenTree* LC_Array::ToGenTree(Compiler* comp, BasicBlock* bb)
return nullptr;
}

//--------------------------------------------------------------------------------------------------
// ToGenTree: Convert a Span.Length operation into a GenTree node.
//
// Arguments:
// comp - Compiler instance to allocate trees
//
// Return Values:
// Returns the gen tree representation for Span.Length
//
GenTree* LC_Span::ToGenTree(Compiler* comp)
{
return comp->gtNewLclvNode(spanIndex->lenLcl, comp->lvaTable[spanIndex->lenLcl].lvType);
}

//--------------------------------------------------------------------------------------------------
// ToGenTree - Convert an "identifier" into a GenTree node.
//
Expand All @@ -138,6 +168,8 @@ GenTree* LC_Ident::ToGenTree(Compiler* comp, BasicBlock* bb)
return comp->gtNewLclvNode(lclNum, comp->lvaTable[lclNum].lvType);
case ArrAccess:
return arrAccess.ToGenTree(comp, bb);
case SpanAccess:
return spanAccess.ToGenTree(comp);
case Null:
return comp->gtNewIconNode(0, TYP_REF);
case ClassHandle:
Expand Down Expand Up @@ -1080,6 +1112,10 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
JitExpandArrayStack<LcOptInfo*>* optInfos = context->GetLoopOptInfo(loop->GetIndex());
assert(optInfos->Size() > 0);

// If we have spans, that means we have to be careful about the stride (see below).
//
bool hasSpans = false;

// We only need to check for iteration behavior if we have array checks.
//
bool checkIterationBehavior = false;
Expand All @@ -1094,6 +1130,11 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
checkIterationBehavior = true;
break;

case LcOptInfo::LcSpan:
checkIterationBehavior = true;
hasSpans = true;
break;

case LcOptInfo::LcTypeTest:
{
LcTypeTestOptInfo* ttInfo = optInfo->AsLcTypeTestOptInfo();
Expand Down Expand Up @@ -1154,23 +1195,37 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
}

const bool isIncreasingLoop = iterInfo->IsIncreasingLoop();
assert(isIncreasingLoop || iterInfo->IsDecreasingLoop());
if (!isIncreasingLoop && !iterInfo->IsDecreasingLoop())
{
// Normally, we reject weird-looking loops in optIsLoopClonable, but it's not the case
Copy link
Member Author

@EgorBo EgorBo Mar 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

A small pre-existing issue, can be reproduced (hits an assert in Checked) in Main via this snippet

Click me 
using System;
using System.Runtime.CompilerServices;

class Program : IDisposable
{
    public static void Main()
    {
        for (int i = 0; i < 1200; i++)
        {
            try
            {
                Test(new int[100000000], 44, new Program());
                Thread.Sleep(16);
            }
            catch
            {
            }
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void Test(int[] arr, int x, IDisposable d)
    {
        for (int i = 0; i < x; i--)
        {
            d.Dispose();
            Console.WriteLine(arr[i]);
        }
    }

    public void Dispose()
    {
    }
}

// when we have both GDVs and array checks inside such loops.
return false;
}

// We already know that this is either increasing or decreasing loop and the
// stride is (> 0) or (< 0). Here, just take the abs() value and check if it
// is beyond the limit.
int stride = abs(iterInfo->IterConst());

if (stride >= 58)
static_assert_no_msg(INT32_MAX >= CORINFO_Array_MaxLength);
if (stride >= (INT32_MAX - (CORINFO_Array_MaxLength - 1) + 1))
{
// Array.MaxLength can have maximum of 0X7FFFFFC7 elements, so make sure
// Array.MaxLength can have maximum of 0x7fffffc7 elements, so make sure
// the stride increment doesn't overflow or underflow the index. Hence,
// the maximum stride limit is set to
// (int.MaxValue - (Array.MaxLength - 1) + 1), which is
// (0X7fffffff - 0x7fffffc7 + 2) = 0x3a or 58.
return false;
}

// We don't know exactly whether we might be dealing with a Span<T> or not,
// but if we suspect we are, we need to be careful about the stride:
// As Span<>.Length can be INT32_MAX unlike arrays.
if (hasSpans && (stride > 1))
{
return false;
}

LC_Ident ident;
// Init conditions
if (iterInfo->HasConstInit)
Expand Down Expand Up @@ -1313,6 +1368,15 @@ bool Compiler::optDeriveLoopCloningConditions(FlowGraphNaturalLoop* loop, LoopCl
context->EnsureArrayDerefs(loop->GetIndex())->Push(array);
}
break;
case LcOptInfo::LcSpan:
{
LcSpanOptInfo* spanInfo = optInfo->AsLcSpanOptInfo();
LC_Span spanLen(&spanInfo->spanIndex);
LC_Ident spanLenIdent = LC_Ident::CreateSpanAccess(spanLen);
LC_Condition cond(opLimitCondition, LC_Expr(ident), LC_Expr(spanLenIdent));
context->EnsureConditions(loop->GetIndex())->Push(cond);
}
break;
case LcOptInfo::LcMdArray:
{
LcMdArrayOptInfo* mdArrInfo = optInfo->AsLcMdArrayOptInfo();
Expand Down Expand Up @@ -1455,10 +1519,6 @@ bool Compiler::optComputeDerefConditions(FlowGraphNaturalLoop* loop, LoopCloneCo
JitExpandArrayStack<LC_Array>* const arrayDeref = context->EnsureArrayDerefs(loop->GetIndex());
JitExpandArrayStack<LC_Ident>* const objDeref = context->EnsureObjDerefs(loop->GetIndex());

// We currently expect to have at least one of these.
//
assert((arrayDeref->Size() != 0) || (objDeref->Size() != 0));

// Generate the array dereference checks.
//
// For each array in the dereference list, construct a tree,
Expand Down Expand Up @@ -1679,6 +1739,39 @@ void Compiler::optPerformStaticOptimizations(FlowGraphNaturalLoop* loop,
DBEXEC(dynamicPath, optDebugLogLoopCloning(arrIndexInfo->arrIndex.useBlock, arrIndexInfo->stmt));
}
break;
case LcOptInfo::LcSpan:
{
LcSpanOptInfo* spanIndexInfo = optInfo->AsLcSpanOptInfo();
compCurBB = spanIndexInfo->spanIndex.useBlock;
GenTree* bndsChkNode = spanIndexInfo->spanIndex.bndsChk;

#ifdef DEBUG
if (verbose)
{
printf("Remove bounds check ");
printTreeID(bndsChkNode->gtGetOp1());
printf(" for " FMT_STMT ", ", spanIndexInfo->stmt->GetID());
spanIndexInfo->spanIndex.Print();
printf(", bounds check nodes: ");
spanIndexInfo->spanIndex.PrintBoundsCheckNode();
printf("\n");
}
#endif // DEBUG

if (bndsChkNode->gtGetOp1()->OperIs(GT_BOUNDS_CHECK))
{
optRemoveCommaBasedRangeCheck(bndsChkNode, spanIndexInfo->stmt);
}
else
{
JITDUMP(" Bounds check already removed\n");

// If the bounds check node isn't there, it better have been converted to a GT_NOP.
assert(bndsChkNode->gtGetOp1()->OperIs(GT_NOP));
}
DBEXEC(dynamicPath, optDebugLogLoopCloning(spanIndexInfo->spanIndex.useBlock, spanIndexInfo->stmt));
}
break;
case LcOptInfo::LcMdArray:
// TODO-CQ: CLONE: Implement.
break;
Expand Down Expand Up @@ -1913,7 +2006,6 @@ BasicBlock* Compiler::optInsertLoopChoiceConditions(LoopCloneContext* contex
BasicBlock* insertAfter)
{
JITDUMP("Inserting loop " FMT_LP " loop choice conditions\n", loop->GetIndex());
assert(context->HasBlockConditions(loop->GetIndex()));
assert(slowPreheader != nullptr);

if (context->HasBlockConditions(loop->GetIndex()))
Expand Down Expand Up @@ -2087,9 +2179,6 @@ void Compiler::optCloneLoop(FlowGraphNaturalLoop* loop, LoopCloneContext* contex
// ...
// slowPreheader --> slowHeader
//
// We should always have block conditions.

assert(context->HasBlockConditions(loop->GetIndex()));

// If any condition is false, go to slowPreheader (which branches or falls through to header of the slow loop).
BasicBlock* slowHeader = nullptr;
Expand Down Expand Up @@ -2272,6 +2361,44 @@ bool Compiler::optExtractArrIndex(GenTree* tree, ArrIndex* result, unsigned lhsN
return true;
}

//---------------------------------------------------------------------------------------------------------------
// optExtractSpanIndex: Try to extract the Span element access from "tree".
//
// Arguments:
// tree - the tree to be checked if it is the Span [] operation.
// result - the extracted information is updated in result.
//
// Return Value:
// Returns true if Span index can be extracted, else, return false.
//
// Notes:
// The way loop cloning works for Span is that we don't actually know (or care)
// if it's a Span or an array, we just extract index and length locals out
/// of the GT_BOUNDS_CHECK node. The fact that the length is a local var
/// allows us to not worry about array/span dereferencing.
//
bool Compiler::optExtractSpanIndex(GenTree* tree, SpanIndex* result)
{
// Bounds checks are almost always wrapped in a comma node
// and are the first operand.
if (!tree->OperIs(GT_COMMA) || !tree->gtGetOp1()->OperIs(GT_BOUNDS_CHECK))
{
return false;
}

GenTreeBoundsChk* arrBndsChk = tree->gtGetOp1()->AsBoundsChk();
if (!arrBndsChk->GetIndex()->OperIs(GT_LCL_VAR) || !arrBndsChk->GetArrayLength()->OperIs(GT_LCL_VAR))
{
return false;
}

result->lenLcl = arrBndsChk->GetArrayLength()->AsLclVarCommon()->GetLclNum();
result->indLcl = arrBndsChk->GetIndex()->AsLclVarCommon()->GetLclNum();
result->bndsChk = tree;
result->useBlock = compCurBB;
return true;
}

//---------------------------------------------------------------------------------------------------------------
// optReconstructArrIndexHelp: Helper function for optReconstructArrIndex. See that function for more details.
//
Expand Down Expand Up @@ -2535,6 +2662,30 @@ Compiler::fgWalkResult Compiler::optCanOptimizeByLoopCloning(GenTree* tree, Loop
return WALK_SKIP_SUBTREES;
}

SpanIndex spanIndex = SpanIndex();
if (info->cloneForArrayBounds && optExtractSpanIndex(tree, &spanIndex))
{
// Check that the span's length local variable is invariant within the loop body.
if (!optIsStackLocalInvariant(info->loop, spanIndex.lenLcl))
{
JITDUMP("Span.Length V%02d is not loop invariant\n", spanIndex.lenLcl);
return WALK_SKIP_SUBTREES;
}

unsigned iterVar = info->context->GetLoopIterInfo(info->loop->GetIndex())->IterVar;
if (spanIndex.indLcl == iterVar)
{
// Update the loop context.
info->context->EnsureLoopOptInfo(info->loop->GetIndex())
->Push(new (this, CMK_LoopOpt) LcSpanOptInfo(spanIndex, info->stmt));
}
else
{
JITDUMP("Induction V%02d is not used as index\n", iterVar);
}
return WALK_SKIP_SUBTREES;
}

if (info->cloneForGDVTests && tree->OperIs(GT_JTRUE))
{
JITDUMP("...GDV considering [%06u]\n", dspTreeID(tree));
Expand Down
Loading
Loading