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.

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

JIT: Establish loop invariant base case based on IR #97182

Merged
merged 7 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 3 additions & 1 deletion src/coreclr/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -1958,7 +1958,7 @@ struct NaturalLoopIterInfo
unsigned IterVar = BAD_VAR_NUM;

#ifdef DEBUG
// Tree that initializes induction varaible outside the loop.
// Tree that initializes induction variable outside the loop.
// Only valid if HasConstInit is true.
GenTree* InitTree = nullptr;
#endif
Expand Down Expand Up @@ -2096,6 +2096,8 @@ class FlowGraphNaturalLoop
void MatchInit(NaturalLoopIterInfo* info, BasicBlock* initBlock, GenTree* init);
bool MatchLimit(unsigned iterVar, GenTree* test, NaturalLoopIterInfo* info);
bool CheckLoopConditionBaseCase(BasicBlock* initBlock, NaturalLoopIterInfo* info);
bool IsZeroTripTest(BasicBlock* initBlock, NaturalLoopIterInfo* info);
bool CondInitBlockEnterSide(BasicBlock* initBlock);
template<typename T>
static bool EvaluateRelop(T op1, T op2, genTreeOps oper);
public:
Expand Down
131 changes: 121 additions & 10 deletions src/coreclr/jit/flowgraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5030,7 +5030,7 @@ bool FlowGraphNaturalLoop::AnalyzeIteration(NaturalLoopIterInfo* info)

if (!CheckLoopConditionBaseCase(initBlock, info))
{
JITDUMP(" Loop condition is not always true\n");
JITDUMP(" Loop condition may not be true on the first iteration\n");
return false;
}

Expand Down Expand Up @@ -5264,11 +5264,10 @@ bool FlowGraphNaturalLoop::EvaluateRelop(T op1, T op2, genTreeOps oper)

//------------------------------------------------------------------------
// CheckLoopConditionBaseCase: Verify that the loop condition is true when the
// loop is entered (or validated immediately on entry).
// loop is entered.
//
// Returns:
// True if we could prove that the condition is true at all interesting
// points in the loop.
// True if we could prove that the condition is true on entry.
//
// Remarks:
// Currently handles the following cases:
Expand Down Expand Up @@ -5307,18 +5306,130 @@ bool FlowGraphNaturalLoop::CheckLoopConditionBaseCase(BasicBlock* initBlock, Nat
}

// Do we have a zero-trip test?
if (initBlock->KindIs(BBJ_COND))
if (initBlock->KindIs(BBJ_COND) && IsZeroTripTest(initBlock, info))
{
GenTree* enteringJTrue = initBlock->lastStmt()->GetRootNode();
assert(enteringJTrue->OperIs(GT_JTRUE));
if (enteringJTrue->gtGetOp1()->OperIsCompare() && ((enteringJTrue->gtGetOp1()->gtFlags & GTF_RELOP_ZTT) != 0))
return true;
}

return false;
}

//------------------------------------------------------------------------
// IsZeroTripTest: Check whether `initBlock`, a BBJ_COND block that enters the
// loop in one case and not in the other, implies that the loop invariant is
// true on entry.
//
// Returns:
// True if we could prove that the loop invariant is true on entry through
// "initBlock".
//
bool FlowGraphNaturalLoop::IsZeroTripTest(BasicBlock* initBlock, NaturalLoopIterInfo* info)
{
assert(initBlock->KindIs(BBJ_COND));
GenTree* enteringJTrue = initBlock->lastStmt()->GetRootNode();
assert(enteringJTrue->OperIs(GT_JTRUE));
GenTree* relop = enteringJTrue->gtGetOp1();
if (!relop->OperIsCmpCompare())
{
return false;
}

// Technically optExtractInitTestIncr only handles the "false"
// entry case, and preheader creation should ensure that that's the
// only time we'll see a BBJ_COND init block. However, it does not
// hurt to let this logic be correct by construction.
bool enterSide = CondInitBlockEnterSide(initBlock);

JITDUMP(" Init block " FMT_BB " enters the loop when condition [%06u] evaluates to %s\n", initBlock->bbNum,
Compiler::dspTreeID(relop), enterSide ? "true" : "false");

GenTree* limitCandidate;
genTreeOps oper;

if (relop->gtGetOp1()->OperIsScalarLocal() && (relop->gtGetOp1()->AsLclVarCommon()->GetLclNum() == info->IterVar))
{
JITDUMP(" op1 is the iteration variable\n");
oper = relop->gtOper;
limitCandidate = relop->gtGetOp2();
}
else if (relop->gtGetOp2()->OperIsScalarLocal() &&
(relop->gtGetOp2()->AsLclVarCommon()->GetLclNum() == info->IterVar))
{
JITDUMP(" op2 is the iteration variable\n");
oper = GenTree::SwapRelop(relop->gtOper);
limitCandidate = relop->gtGetOp1();
}
else
{
JITDUMP(" Relop does not involve iteration variable\n");
return false;
}

if (!enterSide)
{
oper = GenTree::ReverseRelop(oper);
}

// Here we want to prove that [iterVar] [oper] [limitCandidate] implies
// [iterVar] [info->IterOper()] [info->Limit()]. Currently we just do the
// simple exact checks, but this could be improved.

if ((relop->IsUnsigned() != info->TestTree->IsUnsigned()) || (oper != info->TestOper()))
{
JITDUMP(" Condition guarantees V%02u %s%s [%06u], but invariant requires V%02u %s%s [%06u]\n", info->IterVar,
relop->IsUnsigned() ? "(uns) " : "", GenTree::OpName(oper), Compiler::dspTreeID(limitCandidate),
info->IterVar, info->TestTree->IsUnsigned() ? "(uns) " : "", GenTree::OpName(info->TestOper()),
Compiler::dspTreeID(info->Limit()));
return false;
}

JITDUMP(" Condition is established before entry at [%06u]\n", Compiler::dspTreeID(enteringJTrue->gtGetOp1()));
return true;
}

//------------------------------------------------------------------------
// CondInitBlockEnterSide: Determine whether a BBJ_COND init block enters the
Copy link
Member

Choose a reason for hiding this comment

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

Naming nit -- from the name I thought initially this was checking for side entries into loops.

Maybe something like InitBlockEntersLoopOnTrue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed it.

// loop in the false or true case.
//
// Parameters:
// initBlock - A BBJ_COND block that is assumed to dominate the loop, and
// only enter the loop in one of the two cases.
//
// Returns:
// True if the loop is entered if the condition evaluates to true; otherwise false.
//
// Remarks:
// Handles only limited cases (optExtractInitTestIncr ensures that we see
// only limited cases).
//
bool FlowGraphNaturalLoop::CondInitBlockEnterSide(BasicBlock* initBlock)
{
assert(initBlock->KindIs(BBJ_COND));

if (initBlock->FalseTargetIs(GetHeader()))
{
return false;
}

if (initBlock->TrueTargetIs(GetHeader()))
{
return true;
}

for (FlowEdge* enterEdge : EntryEdges())
Copy link
Member

Choose a reason for hiding this comment

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

If this is handling a preheader, can you add a comment? Can do this in a follow-up.

Also we'd expect that entering is BBJ_ALWAYS

The check in optExtractInitTestIncr is still using bbFallsThrough so I wonder if we're missing some cases from that.

Copy link
Member Author

Choose a reason for hiding this comment

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

If this is handling a preheader, can you add a comment? Can do this in a follow-up.

Also we'd expect that entering is BBJ_ALWAYS

The check in optExtractInitTestIncr is still using bbFallsThrough so I wonder if we're missing some cases from that.

Currently FlowGraphNaturalLoop::AnalyzeIteration is general enough to be used before preheaders have been created. We don't actually use it during that, but still I kept this similarly general (and matching optExtractInitTestIncr).
We could probably clean up these two methods at the same time. I agree we should also generalize optExtractInitTestIncr, in particular to handle loops we did not invert. We could also consider using dominators to try harder to prove the "loop invariant" to be true when initially entered, since I think we usually have them available in the places that call FlowGraphNaturalLoop::AnalyzeIteration, so we could do something similar to RBO here.

I'll add a comment. I also just noticed that I forgot the equality check on the limits, so need to add that too.

{
BasicBlock* entering = enterEdge->getSourceBlock();
if (initBlock->FalseTargetIs(entering))
{
return false;
}
if (initBlock->TrueTargetIs(entering))
{
JITDUMP(" Condition is established before entry at [%06u]\n",
Compiler::dspTreeID(enteringJTrue->gtGetOp1()));
return true;
}
}

assert(!"Could not find init block enter side");
return false;
}

Expand Down
1 change: 0 additions & 1 deletion src/coreclr/jit/gentree.h
Original file line number Diff line number Diff line change
Expand Up @@ -503,7 +503,6 @@ enum GenTreeFlags : unsigned int

GTF_RELOP_NAN_UN = 0x80000000, // GT_<relop> -- Is branch taken if ops are NaN?
GTF_RELOP_JMP_USED = 0x40000000, // GT_<relop> -- result of compare used for jump or ?:
GTF_RELOP_ZTT = 0x08000000, // GT_<relop> -- Loop test cloned for converting while-loops into do-while
// with explicit "loop test" in the header block.

GTF_RET_MERGED = 0x80000000, // GT_RETURN -- This is a return generated during epilog merging.
Expand Down
11 changes: 0 additions & 11 deletions src/coreclr/jit/loopcloning.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1841,17 +1841,6 @@ bool Compiler::optIsLoopClonable(FlowGraphNaturalLoop* loop, LoopCloneContext* c
return false;
}

// TODO-Quirk: Can be removed, loop invariant is validated by
// `FlowGraphNaturalLoop::AnalyzeIteration`.
if (!iterInfo->TestTree->OperIsCompare() || ((iterInfo->TestTree->gtFlags & GTF_RELOP_ZTT) == 0))
{
JITDUMP("Loop cloning: rejecting loop " FMT_LP
". Loop inversion NOT present, loop test [%06u] may not protect "
"entry from head.\n",
loop->GetIndex(), iterInfo->TestTree->gtTreeID);
return false;
}

#ifdef DEBUG
const unsigned ivLclNum = iterInfo->IterVar;
GenTree* const op1 = iterInfo->Iterator();
Expand Down
5 changes: 0 additions & 5 deletions src/coreclr/jit/optimizer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2154,11 +2154,6 @@ bool Compiler::optInvertWhileLoop(BasicBlock* block)
assert(originalCompareTree->OperIsCompare());
assert(clonedCompareTree->OperIsCompare());

// Flag compare and cloned copy so later we know this loop
// has a proper zero trip test.
originalCompareTree->gtFlags |= GTF_RELOP_ZTT;
clonedCompareTree->gtFlags |= GTF_RELOP_ZTT;

// The original test branches to remain in the loop. The
// new cloned test will branch to avoid the loop. So the
// cloned compare needs to reverse the branch condition.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ public class Runtime_95315
[Fact]
public static void TestEntryPoint()
{
// Make sure 'Bar' tiers up completely...
for (int i = 0; i < 4; i++)
{
for (int j = 0; j < 200; j++)
Expand Down
Loading