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

Jump to bottom

[indvars] Missing variables at Og: #69920

Merged
merged 9 commits into from
Apr 8, 2024
Merged

[indvars] Missing variables at Og: #69920

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fdd13d9
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Oct 23, 2023
a6260d4
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Oct 26, 2023
a27cef0
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Dec 8, 2023
5935de9
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Jan 9, 2024
9941c3a
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Jan 9, 2024
bf5b9bc
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Jan 16, 2024
0427224
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Mar 13, 2024
ac8a3aa
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Mar 18, 2024
9e4c629
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Mar 19, 2024
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
24 changes: 24 additions & 0 deletions llvm/include/llvm/Support/GenericLoopInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@
#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetOperations.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/ValueHandle.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/GenericDomTree.h"

Expand Down Expand Up @@ -75,6 +77,13 @@ template <class BlockT, class LoopT> class LoopBase {
const LoopBase<BlockT, LoopT> &
operator=(const LoopBase<BlockT, LoopT> &) = delete;

// Induction variable exit value and its debug users, preserved by the
// 'indvars' pass, when it detects that the loop can be deleted and the
// there are no PHIs to be rewritten.
// For now, we only preserve single induction variables.
Value *IndVarFinalValue = nullptr;
SmallVector<WeakVH> IndVarDebugUsers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it necessary for these to be members on the Loop object itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

It also introduced some link errors. Reverting the patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reverting. I will look at that layering violation.


public:
/// Return the nesting level of this loop. An outer-most loop has depth 1,
/// for consistency with loop depth values used for basic blocks, where depth
Expand Down Expand Up @@ -251,6 +260,21 @@ template <class BlockT, class LoopT> class LoopBase {
[&](BlockT *Pred) { return contains(Pred); });
}

/// Preserve the induction variable exit value and its debug users by the
/// 'indvars' pass if the loop can deleted. Those debug users will be used
/// by the 'loop-delete' pass.
void preserveDebugInductionVariableInfo(
Value *FinalValue, SmallVector<DbgVariableIntrinsic *> DbgUsers) {
IndVarFinalValue = FinalValue;
for (DbgVariableIntrinsic *DebugUser : DbgUsers)
IndVarDebugUsers.push_back(DebugUser);
}

Value *getDebugInductionVariableFinalValue() { return IndVarFinalValue; }
SmallVector<WeakVH> &getDebugInductionVariableDebugUsers() {
return IndVarDebugUsers;
}

//===--------------------------------------------------------------------===//
// APIs for simple analysis of the loop.
//
Expand Down
6 changes: 6 additions & 0 deletions llvm/include/llvm/Transforms/Utils/LoopUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,12 @@ int rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
ReplaceExitVal ReplaceExitValue,
SmallVector<WeakTrackingVH, 16> &DeadInsts);

/// Assign exit values to variables that use this loop variable during the loop.
Copy link
Member

Choose a reason for hiding this comment

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

I think for a few of these function signatures it won't be obvious to the reader how to use them -- could you elaborate the documentation a little, I reckon it only needs a sentence for each function, but the sentences need to refer to the arguments. See the documentation for setProfileInfoAfterUnrolling below for inspiration, something like "For variable-users of \p IndVar create assignments in \p Successor of the exit value \p PN" or similar.

void addDebugValuesToIncomingValue(BasicBlock *Successor, Value *IndVar,
PHINode *PN);
void addDebugValuesToLoopVariable(BasicBlock *Successor, Value *ExitValue,
PHINode *PN);

/// Set weights for \p UnrolledLoop and \p RemainderLoop based on weights for
/// \p OrigLoop and the following distribution of \p OrigLoop iteration among \p
/// UnrolledLoop and \p RemainderLoop. \p UnrolledLoop receives weights that
Expand Down
65 changes: 65 additions & 0 deletions llvm/lib/Transforms/Utils/LoopUtils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include "llvm/Analysis/ScalarEvolutionAliasAnalysis.h"
#include "llvm/Analysis/ScalarEvolutionExpressions.h"
#include "llvm/IR/DIBuilder.h"
#include "llvm/IR/DebugInfo.h"
#include "llvm/IR/Dominators.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicInst.h"
Expand Down Expand Up @@ -607,6 +608,17 @@ void llvm::deleteDeadLoop(Loop *L, DominatorTree *DT, ScalarEvolution *SE,
llvm::SmallVector<DPValue *, 4> DeadDPValues;

if (ExitBlock) {
if (ExitBlock->phis().empty()) {
// As the loop is deleted, replace the debug users with the preserved
// induction variable final value recorded by the 'indvar' pass.
Value *FinalValue = L->getDebugInductionVariableFinalValue();
SmallVector<WeakVH> &DbgUsers = L->getDebugInductionVariableDebugUsers();
for (WeakVH &DebugUser : DbgUsers)
if (DebugUser)
dyn_cast<DbgVariableIntrinsic>(DebugUser)->replaceVariableLocationOp(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dyn_cast<DbgVariableIntrinsic>(DebugUser)->replaceVariableLocationOp(
cast<DbgVariableIntrinsic>(DebugUser)->replaceVariableLocationOp(

This should just be a normal cast - the idea of dyn_cast is that it can return nullptr if the cast is unsuccessful, unlike cast which just asserts; if you aren't checking the result for nullptr, then there's no value in using dyn_cast over cast. I think in this case, you just want cast, since the check above already verifies that the value has not been deleted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the additional information on dyn_cast. Changed to cast.

0u, FinalValue);
}

// Given LCSSA form is satisfied, we should not have users of instructions
// within the dead loop outside of the loop. However, LCSSA doesn't take
// unreachable uses into account. We handle them here.
Expand Down Expand Up @@ -1412,6 +1424,36 @@ static bool checkIsIndPhi(PHINode *Phi, Loop *L, ScalarEvolution *SE,
return InductionDescriptor::isInductionPHI(Phi, L, SE, ID);
}

void llvm::addDebugValuesToIncomingValue(BasicBlock *Successor, Value *IndVar,
PHINode *PN) {
SmallVector<DbgVariableIntrinsic *> DbgUsers;
findDbgUsers(DbgUsers, IndVar);
for (auto *DebugUser : DbgUsers) {
// Skip debug-users with variadic variable locations; they will not,
// get updated, which is fine as that is the existing behaviour.
if (DebugUser->hasArgList())
continue;
auto *Cloned = cast<DbgVariableIntrinsic>(DebugUser->clone());
Cloned->replaceVariableLocationOp(0u, PN);
Cloned->insertBefore(Successor->getFirstNonPHI());
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fix this up for the new insert logic (same in the function below):

Suggested change
Cloned->insertBefore(Successor->getFirstNonPHI());
Cloned->insertBefore(Successor, Successor->getFirstNonPHIIt());

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to use new insert logic.

}
}

void llvm::addDebugValuesToLoopVariable(BasicBlock *Successor, Value *ExitValue,
PHINode *PN) {
SmallVector<DbgVariableIntrinsic *> DbgUsers;
findDbgUsers(DbgUsers, PN);
for (auto *DebugUser : DbgUsers) {
// Skip debug-users with variadic variable locations; they will not,
// get updated, which is fine as that is the existing behaviour.
if (DebugUser->hasArgList())
continue;
auto *Cloned = cast<DbgVariableIntrinsic>(DebugUser->clone());
Cloned->replaceVariableLocationOp(0u, ExitValue);
Cloned->insertBefore(Successor->getFirstNonPHI());
}
}

int llvm::rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
ScalarEvolution *SE,
const TargetTransformInfo *TTI,
Expand Down Expand Up @@ -1553,6 +1595,10 @@ int llvm::rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
(isa<PHINode>(Inst) || isa<LandingPadInst>(Inst)) ?
&*Inst->getParent()->getFirstInsertionPt() : Inst;
RewritePhiSet.emplace_back(PN, i, ExitValue, InsertPt, HighCost);

// Add debug values for the candidate PHINode incoming value.
if (BasicBlock *Successor = ExitBB->getSingleSuccessor())
addDebugValuesToIncomingValue(Successor, PN->getIncomingValue(i), PN);
}
}
}
Expand Down Expand Up @@ -1611,11 +1657,30 @@ int llvm::rewriteLoopExitValues(Loop *L, LoopInfo *LI, TargetLibraryInfo *TLI,
// Replace PN with ExitVal if that is legal and does not break LCSSA.
if (PN->getNumIncomingValues() == 1 &&
LI->replacementPreservesLCSSAForm(PN, ExitVal)) {
addDebugValuesToLoopVariable(PN->getParent(), ExitVal, PN);
PN->replaceAllUsesWith(ExitVal);
PN->eraseFromParent();
}
}

// If the loop can be deleted and there are no PHIs to be rewritten (there
// are no loop live-out values), record debug variables corresponding to the
// induction variable with their constant exit-values. Those values will be
// inserted by the 'deletion loop' logic.
if (LoopCanBeDel && RewritePhiSet.empty()) {
if (auto *IndVar = L->getInductionVariable(*SE)) {
const SCEV *PNSCEV = SE->getSCEVAtScope(IndVar, L->getParentLoop());
if (auto *Const = dyn_cast<SCEVConstant>(PNSCEV)) {
Value *FinalIVValue = Const->getValue();
if (L->getUniqueExitBlock()) {
SmallVector<DbgVariableIntrinsic *> DbgUsers;
findDbgUsers(DbgUsers, IndVar);
L->preserveDebugInductionVariableInfo(FinalIVValue, DbgUsers);
}
}
}
}

// The insertion point instruction may have been deleted; clear it out
// so that the rewriter doesn't trip over it later.
Rewriter.clearInsertPoint();
Expand Down
129 changes: 129 additions & 0 deletions llvm/test/Transforms/IndVarSimplify/pr51735-1.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
; RUN: opt -passes="loop(indvars)" \
; RUN: --experimental-debuginfo-iterators=false -S -o - < %s | \
; RUN: FileCheck --check-prefix=CHECK %s
; RUN: opt -passes="loop(indvars,loop-deletion)" \
; RUN: --experimental-debuginfo-iterators=false -S -o - < %s | \
; RUN: FileCheck --check-prefix=CHECK %s

; Make sure that when we delete the loop, that the variable Index has
; the 777 value.

; As this test case does fire the 'indvars' transformation, the debug values
; are added to the 'for.end' exit block. No debug values are preserved by the
; pass to be used by the 'loop-deletion' pass.

; CHECK: for.cond:
; CHECK: call void @llvm.dbg.value(metadata i32 %Index.0, metadata ![[DBG:[0-9]+]], {{.*}}

; CHECK: for.extra:
; CHECK: %call.0 = call noundef i32 @"?nop@@YAHH@Z"(i32 noundef %Index.0), {{.*}}
; CHECK: br i1 %cmp.0, label %for.cond, label %if.else, {{.*}}

; CHECK: if.then:
; CHECK: call void @llvm.dbg.value(metadata i32 777, metadata ![[DBG]], {{.*}}
; CHECK: call void @llvm.dbg.value(metadata i32 %Var.1, metadata ![[VAR:[0-9]+]], {{.*}}
; CHECK: br label %for.end, {{.*}}

; CHECK: if.else:
; CHECK: call void @llvm.dbg.value(metadata i32 %Var.2, metadata ![[VAR:[0-9]+]], {{.*}}
; CHECK: br label %for.end, {{.*}}

; CHECK: for.end:
; CHECK: call void @llvm.dbg.value(metadata i32 777, metadata ![[DBG]], {{.*}}

; CHECK-DAG: ![[DBG]] = !DILocalVariable(name: "Index"{{.*}})
; CHECK-DAG: ![[VAR]] = !DILocalVariable(name: "Var"{{.*}})

define dso_local noundef i32 @"?nop@@YAHH@Z"(i32 noundef %Param) !dbg !11 {
entry:
%Param.addr = alloca i32, align 4
store i32 %Param, ptr %Param.addr, align 4
call void @llvm.dbg.declare(metadata ptr %Param.addr, metadata !32, metadata !DIExpression()), !dbg !35
ret i32 0, !dbg !36
}

define dso_local void @_Z3barv() local_unnamed_addr #1 !dbg !12 {
entry:
call void @llvm.dbg.value(metadata i32 777, metadata !16, metadata !DIExpression()), !dbg !17
call void @llvm.dbg.value(metadata i32 27, metadata !18, metadata !DIExpression()), !dbg !17
call void @llvm.dbg.value(metadata i32 1, metadata !19, metadata !DIExpression()), !dbg !17
call void @llvm.dbg.value(metadata i32 1, metadata !30, metadata !DIExpression()), !dbg !17
br label %for.cond, !dbg !20

for.cond: ; preds = %for.cond, %entry
%Index.0 = phi i32 [ 27, %entry ], [ %inc, %for.extra ], !dbg !17
call void @llvm.dbg.value(metadata i32 %Index.0, metadata !18, metadata !DIExpression()), !dbg !17
%cmp = icmp ult i32 %Index.0, 777, !dbg !21
%inc = add nuw nsw i32 %Index.0, 1, !dbg !24
call void @llvm.dbg.value(metadata i32 %inc, metadata !18, metadata !DIExpression()), !dbg !17
br i1 %cmp, label %for.extra, label %if.then, !dbg !25, !llvm.loop !26

for.extra:
%call.0 = call noundef i32 @"?nop@@YAHH@Z"(i32 noundef %Index.0), !dbg !21
%cmp.0 = icmp ult i32 %Index.0, %call.0, !dbg !21
br i1 %cmp.0, label %for.cond, label %if.else, !dbg !25, !llvm.loop !26

if.then: ; preds = %for.cond
%Var.1 = add nsw i32 %Index.0, 1, !dbg !20
call void @llvm.dbg.value(metadata i32 %Var.1, metadata !19, metadata !DIExpression()), !dbg !20
br label %for.end, !dbg !20

if.else:
%Var.2 = add nsw i32 %Index.0, 2, !dbg !20
call void @llvm.dbg.value(metadata i32 %Var.2, metadata !19, metadata !DIExpression()), !dbg !20
br label %for.end, !dbg !20

for.end: ; preds = %if.else, %if.then
%Zeta.0 = phi i32 [ %Var.1, %if.then ], [ %Var.2, %if.else ], !dbg !20
call void @llvm.dbg.value(metadata i32 %Zeta.0, metadata !30, metadata !DIExpression()), !dbg !20
%Var.3 = add nsw i32 %Index.0, 1, !dbg !20
call void @llvm.dbg.value(metadata i32 %Var.3, metadata !19, metadata !DIExpression()), !dbg !20
%call = call noundef i32 @"?nop@@YAHH@Z"(i32 noundef %Index.0), !dbg !37
ret void, !dbg !29
}

declare void @llvm.dbg.value(metadata, metadata, metadata)
declare void @llvm.dbg.declare(metadata, metadata, metadata)

!llvm.dbg.cu = !{!0}
!llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
!llvm.ident = !{!9}

!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "test.cpp", directory: "")
!2 = !{i32 7, !"Dwarf Version", i32 5}
!3 = !{i32 2, !"Debug Info Version", i32 3}
!4 = !{i32 1, !"wchar_size", i32 4}
!5 = !{i32 8, !"PIC Level", i32 2}
!6 = !{i32 7, !"PIE Level", i32 2}
!7 = !{i32 7, !"uwtable", i32 2}
!8 = !{i32 7, !"frame-pointer", i32 2}
!9 = !{!"clang version 18.0.0"}
!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!11 = distinct !DISubprogram(name: "nop", linkageName: "?nop@@YAHH@Z", scope: !1, file: !1, line: 1, type: !33, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !31)
!12 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !1, file: !1, line: 5, type: !13, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !15)
!13 = !DISubroutineType(types: !14)
!14 = !{null}
!15 = !{}
!16 = !DILocalVariable(name: "End", scope: !12, file: !1, line: 6, type: !10)
!17 = !DILocation(line: 0, scope: !12)
!18 = !DILocalVariable(name: "Index", scope: !12, file: !1, line: 7, type: !10)
!19 = !DILocalVariable(name: "Var", scope: !12, file: !1, line: 8, type: !10)
!20 = !DILocation(line: 9, column: 3, scope: !12)
!21 = !DILocation(line: 9, column: 16, scope: !22)
!22 = distinct !DILexicalBlock(scope: !23, file: !1, line: 9, column: 3)
!23 = distinct !DILexicalBlock(scope: !12, file: !1, line: 9, column: 3)
!24 = !DILocation(line: 9, column: 23, scope: !22)
!25 = !DILocation(line: 9, column: 3, scope: !23)
!26 = distinct !{!26, !25, !27, !28}
!27 = !DILocation(line: 10, column: 5, scope: !23)
!28 = !{!"llvm.loop.mustprogress"}
!29 = !DILocation(line: 12, column: 1, scope: !12)
!30 = !DILocalVariable(name: "Zeta", scope: !12, file: !1, line: 8, type: !10)
!31 = !{!32}
!32 = !DILocalVariable(name: "Param", arg: 1, scope: !11, file: !1, line: 1, type: !10)
!33 = !DISubroutineType(types: !34)
!34 = !{!10, !10}
!35 = !DILocation(line: 1, scope: !11)
!36 = !DILocation(line: 2, scope: !11)
!37 = !DILocation(line: 20, scope: !12)
Loading
Loading