Skip to content

[LoopInterchange] Hoist isComputableLoopNest() in the control flow #124247

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
merged 6 commits into from
Feb 5, 2025
Merged

[LoopInterchange] Hoist isComputableLoopNest() in the control flow #124247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fad4645
[LoopInterchange] Hoist isCompuatableLoopNest() in the control flow
madhur13490 Jan 22, 2025
1d9e54a
Update tests to account for new remarks
madhur13490 Jan 28, 2025
7e2710c
Address formatting issues
madhur13490 Jan 28, 2025
31f9942
Address review comments
madhur13490 Jan 29, 2025
1e95541
Change to RemarksAnalysis
madhur13490 Jan 30, 2025
d351384
Add simplified test
madhur13490 Feb 4, 2025
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
59 changes: 35 additions & 24 deletions llvm/lib/Transforms/Scalar/LoopInterchange.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,27 @@ static bool hasSupportedLoopDepth(SmallVectorImpl<Loop *> &LoopList,
}
return true;
}

static bool isComputableLoopNest(ScalarEvolution *SE,
ArrayRef<Loop *> LoopList) {
for (Loop *L : LoopList) {
const SCEV *ExitCountOuter = SE->getBackedgeTakenCount(L);
if (isa<SCEVCouldNotCompute>(ExitCountOuter)) {
LLVM_DEBUG(dbgs() << "Couldn't compute backedge count\n");
return false;
}
if (L->getNumBackEdges() != 1) {
LLVM_DEBUG(dbgs() << "NumBackEdges is not equal to 1\n");
return false;
}
if (!L->getExitingBlock()) {
LLVM_DEBUG(dbgs() << "Loop doesn't have unique exit block\n");
return false;
}
}
return true;
}

namespace {

/// LoopInterchangeLegality checks if it is legal to interchange the loop.
Expand Down Expand Up @@ -431,25 +452,6 @@ struct LoopInterchange {
return processLoopList(LoopList);
}

bool isComputableLoopNest(ArrayRef<Loop *> LoopList) {
for (Loop *L : LoopList) {
const SCEV *ExitCountOuter = SE->getBackedgeTakenCount(L);
if (isa<SCEVCouldNotCompute>(ExitCountOuter)) {
LLVM_DEBUG(dbgs() << "Couldn't compute backedge count\n");
return false;
}
if (L->getNumBackEdges() != 1) {
LLVM_DEBUG(dbgs() << "NumBackEdges is not equal to 1\n");
return false;
}
if (!L->getExitingBlock()) {
LLVM_DEBUG(dbgs() << "Loop doesn't have unique exit block\n");
return false;
}
}
return true;
}

unsigned selectLoopForInterchange(ArrayRef<Loop *> LoopList) {
// TODO: Add a better heuristic to select the loop to be interchanged based
// on the dependence matrix. Currently we select the innermost loop.
Expand All @@ -464,10 +466,6 @@ struct LoopInterchange {
"Unsupported depth of loop nest.");

unsigned LoopNestDepth = LoopList.size();
if (!isComputableLoopNest(LoopList)) {
LLVM_DEBUG(dbgs() << "Not valid loop candidate for interchange\n");
return false;
}

LLVM_DEBUG(dbgs() << "Processing LoopList of size = " << LoopNestDepth
<< "\n");
Expand Down Expand Up @@ -1761,10 +1759,23 @@ PreservedAnalyses LoopInterchangePass::run(LoopNest &LN,
// Ensure minimum depth of the loop nest to do the interchange.
if (!hasSupportedLoopDepth(LoopList, ORE))
return PreservedAnalyses::all();
// Ensure computable loop nest.
if (!isComputableLoopNest(&AR.SE, LoopList)) {
LLVM_DEBUG(dbgs() << "Not valid loop candidate for interchange\n");
return PreservedAnalyses::all();
}

ORE.emit([&]() {
return OptimizationRemarkAnalysis(DEBUG_TYPE, "Dependence",
LN.getOutermostLoop().getStartLoc(),
LN.getOutermostLoop().getHeader())
<< "Computed dependence info, invoking the transform.";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a user-facing optimisation remark, but it is quite "technical".
Correct me if I am wrong, but I think what we are trying to say here is equivalent to this:

LLVM_DEBUG(dbgs() << "Loops are legal to interchange\n");

in function processLoop on line 525.

My question is: can we turn this debug message into an OptimisationRemark and not need this new opt remark? I think this could serve two purposes: it is actually interesting information for users, and does it help you with knowing that "dep info has been computed, the transformation is legal"?

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed above, I think this message is just for testing that the process exits before computing dependence info if the loop is not computable. To achieve this, inserting a new message here seems to be reasonable to me.
However, in terms of messages to users, I think this is a bit excessive or noisy. Replacing it with dbgs() would be one option. It depends on which what we want to prioritize: test coverage or having useful "technical" messages for users.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering if the state that we are trying to capture here (interchange is legal, but unknown if profitable) is caught by the "Loops are legal to interchange" debug message so that we can change that in an optimisation remark and don't need this new one.

Alternatively, I don't mind changing this optimisation remark in a debug message. In general I don't like tests that rely on debug builds, but this is an exception that would be okay I think.

But I don't have strong opinions on this to be honest, so WDYIT @madhur13490 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for chipping in many times, but I don't think what we want to capture here is "interchange is legal", I think it is something like "This loop form is not supported, so we will not continue the following processes (including the legality check)". Let me know if I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree with @kasuga-fj. At this point in the code, we cannot guarantee that the loop nest is legal. For example, if Outer most loop does not have unique exit then transform can terminate. (line 489 )

(I do plan to pull that check up too, but that is of lower priority.)

});

DependenceInfo DI(&F, &AR.AA, &AR.SE, &AR.LI);
std::unique_ptr<CacheCost> CC =
CacheCost::getCacheCost(LN.getOutermostLoop(), AR, DI);

if (!LoopInterchange(&AR.SE, &AR.LI, &DI, &AR.DT, CC, &ORE).run(LN))
return PreservedAnalyses::all();
U.markLoopNestChanged(true);
Expand Down
48 changes: 48 additions & 0 deletions llvm/test/Transforms/LoopInterchange/loop-interchange-optimization-remarks.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,14 @@ for.end19:
ret void
}

; CHECK: --- !Analysis
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Dependence
; CHECK-NEXT: Function: test01
; CHECK-NEXT: Args:
; CHECK-NEXT: - String: Computed dependence info, invoking the transform.
; CHECK-NEXT: ...

; CHECK: --- !Missed
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Dependence
Expand All @@ -66,6 +74,14 @@ for.end19:
; CHECK-NEXT: - String: Cannot interchange loops due to dependences.
; CHECK-NEXT: ...

; DELIN: --- !Analysis
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: Dependence
; DELIN-NEXT: Function: test01
; DELIN-NEXT: Args:
; DELIN-NEXT: - String: Computed dependence info, invoking the transform.
; DELIN-NEXT: ...

; DELIN: --- !Missed
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: InterchangeNotProfitable
Expand Down Expand Up @@ -118,6 +134,14 @@ define void @test02(i32 %k, i32 %N) {
ret void
}

; CHECK: --- !Analysis
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Dependence
; CHECK-NEXT: Function: test02
; CHECK-NEXT: Args:
; CHECK-NEXT: - String: Computed dependence info, invoking the transform.
; CHECK-NEXT: ...

; CHECK: --- !Missed
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Dependence
Expand All @@ -126,6 +150,14 @@ define void @test02(i32 %k, i32 %N) {
; CHECK-NEXT: - String: Cannot interchange loops due to dependences.
; CHECK-NEXT: ...

; DELIN: --- !Analysis
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: Dependence
; DELIN-NEXT: Function: test02
; DELIN-NEXT: Args:
; DELIN-NEXT: - String: Computed dependence info, invoking the transform.
; DELIN-NEXT: ...

; DELIN: --- !Passed
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: Interchanged
Expand Down Expand Up @@ -174,6 +206,14 @@ for.body4: ; preds = %for.body4, %for.con
br i1 %exitcond, label %for.body4, label %for.cond.loopexit
}

; CHECK: --- !Analysis
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Dependence
; CHECK-NEXT: Function: test03
; CHECK-NEXT: Args:
; CHECK-NEXT: - String: Computed dependence info, invoking the transform.
; CHECK-NEXT: ...

; CHECK: --- !Passed
; CHECK-NEXT: Pass: loop-interchange
; CHECK-NEXT: Name: Interchanged
Expand All @@ -182,6 +222,14 @@ for.body4: ; preds = %for.body4, %for.con
; CHECK-NEXT: - String: Loop interchanged with enclosing loop.
; CHECK-NEXT: ...

; DELIN: --- !Analysis
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: Dependence
; DELIN-NEXT: Function: test03
; DELIN-NEXT: Args:
; DELIN-NEXT: - String: Computed dependence info, invoking the transform.
; DELIN-NEXT: ...

; DELIN: --- !Passed
; DELIN-NEXT: Pass: loop-interchange
; DELIN-NEXT: Name: Interchanged
Expand Down
52 changes: 52 additions & 0 deletions llvm/test/Transforms/LoopInterchange/no-dependence-info.ll
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a brief comment describing what this test checks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
; RUN: opt %s -passes='loop-interchange' -pass-remarks=loop-interchange -disable-output 2>&1 | FileCheck --allow-empty %s

target triple = "aarch64-unknown-linux-gnu"

; CHECK-NOT: Computed dependence info, invoking the transform.

; For the below test, backedge count cannot be computed.
; Computing backedge count requires only SCEV and should
; not require dependence info.
;
; void bar(int m, int n) {
; for (unsigned int i = 0; i < m; ++i) {
; for (unsigned int j = 0; j < m; ++j) {
; // dummy code
; }
; }
;}

define void @bar(i32 %m, i32 %n)
{
entry:
br label %outer.header

outer.header:
%m_temp1 = phi i32 [%m, %entry], [%m_temp, %outer.latch]
br label %inner.header


inner.header:
%n_temp1 = phi i32 [%n, %outer.header], [%n_temp, %inner.latch]

br label %body

body:
; dummy code

br label %inner.latch

inner.latch:
%n_temp = add i32 %n_temp1, 1
%cmp2 = icmp eq i32 %n_temp, 1
br i1 %cmp2, label %outer.latch, label %inner.header

outer.latch:
%m_temp = add i32 %n, 1
%cmp3 = icmp eq i32 %m_temp, 1
br i1 %cmp3, label %exit, label %outer.header

exit:
ret void
}

8 changes: 8 additions & 0 deletions llvm/test/Transforms/LoopInterchange/pr43326-ideal-access-pattern.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,14 @@
; }
; }

; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: pr43326-triply-nested
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Passed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Interchanged
Expand Down
8 changes: 8 additions & 0 deletions llvm/test/Transforms/LoopInterchange/pr43326.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@
@d = global i32 0
@e = global [1 x [1 x i32]] zeroinitializer

; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: pr43326
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Passed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Interchanged
Expand Down
8 changes: 8 additions & 0 deletions llvm/test/Transforms/LoopInterchange/pr48212.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,14 @@
; RUN: -verify-dom-info -verify-loop-info -verify-loop-lcssa 2>&1
; RUN: FileCheck --input-file=%t --check-prefix=REMARKS %s

; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: pr48212
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Passed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Interchanged
Expand Down
56 changes: 56 additions & 0 deletions llvm/test/Transforms/LoopInterchange/reductions-across-inner-and-outer-loop.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,14 @@

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"

; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test1
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Passed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Interchanged
Expand Down Expand Up @@ -77,6 +85,14 @@ for1.loopexit: ; preds = %for1.inc

; In this test case, the inner reduction PHI %inner does not involve the outer
; reduction PHI %sum.outer, do not interchange.
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test2
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Missed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: UnsupportedPHIOuter
Expand Down Expand Up @@ -114,6 +130,14 @@ for1.loopexit: ; preds = %for1.inc

; Check that we do not interchange if there is an additional instruction
; between the outer and inner reduction PHIs.
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test3
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Missed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: UnsupportedPHIOuter
Expand Down Expand Up @@ -151,6 +175,14 @@ for1.loopexit: ; preds = %for1.inc
}

; Check that we do not interchange if reduction is stored in an invariant address inside inner loop
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test4
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Missed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
Expand Down Expand Up @@ -190,6 +222,14 @@ for1.loopexit: ; preds = %for1.inc

; Check that we do not interchange or crash if the PHI in the outer loop gets a
; constant from the inner loop.
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test_constant_inner_loop_res
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Missed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: UnsupportedPHIOuter
Expand Down Expand Up @@ -229,6 +269,14 @@ for1.loopexit: ; preds = %for1.inc

; Floating point reductions are interchanged if all the fp instructions
; involved allow reassociation.
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test5
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Passed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Interchanged
Expand Down Expand Up @@ -269,6 +317,14 @@ for.exit: ; preds = %outer.inc

; Floating point reductions are not interchanged if not all the fp instructions
; involved allow reassociation.
; REMARKS: --- !Analysis
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: Dependence
; REMARKS-NEXT: Function: test6
; REMARKS-NEXT: Args:
; REMARKS-NEXT: - String: Computed dependence info, invoking the transform.
; REMARKS-NEXT: ...

; REMARKS: --- !Missed
; REMARKS-NEXT: Pass: loop-interchange
; REMARKS-NEXT: Name: UnsupportedPHIOuter
Expand Down