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

[indvars] Missing variables at Og #88270

Merged
merged 12 commits into from
May 22, 2024
Merged

[indvars] Missing variables at Og #88270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 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
6187569
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Apr 10, 2024
0d06af2
[indvars] Missing variables at Og:
CarlosAlbertoEnciso Apr 11, 2024
a1014a9
[indvars] Missing variables at Og:
CarlosAlbertoEnciso May 22, 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
25 changes: 25 additions & 0 deletions llvm/include/llvm/Analysis/LoopInfo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
#include "llvm/ADT/SmallVector.h"
#include "llvm/IR/CFG.h"
#include "llvm/IR/Instructions.h"
#include "llvm/IR/IntrinsicInst.h"
#include "llvm/IR/PassManager.h"
#include "llvm/IR/ValueHandle.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding these includes to a core header like LoopInfo.h adds significant compile-time overhead.

#include "llvm/Pass.h"
#include "llvm/Support/GenericLoopInfo.h"
#include <algorithm>
Expand Down Expand Up @@ -392,13 +394,36 @@ class LLVM_EXTERNAL_VISIBILITY Loop : public LoopBase<BasicBlock, Loop> {
return "<unnamed loop>";
}

/// 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,
const SmallVectorImpl<DbgVariableIntrinsic *> &DbgUsers) {
IndVarFinalValue = FinalValue;
for (auto &DebugUser : DbgUsers)
IndVarDebugUsers.push_back(DebugUser);
}

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

private:
Loop() = default;

friend class LoopInfoBase<BasicBlock, Loop>;
friend class LoopBase<BasicBlock, Loop>;
explicit Loop(BasicBlock *BB) : LoopBase<BasicBlock, Loop>(BB) {}
~Loop() = default;

// 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;
};

// Implementation in Support/GenericLoopInfoImpl.h
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.
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)
cast<DbgVariableIntrinsic>(DebugUser)->replaceVariableLocationOp(
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, Successor->getFirstNonPHIIt());
}
}

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, Successor->getFirstNonPHIIt());
}
}

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that we can't insert a dbg.value in the exit block right now, here, rather than waiting for loop-deletion to delete the loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

@OCHyams Sorry for my delay in answering your question.

  // 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 (L->getUniqueExitBlock()) {
          ...
        }
  }

The RewritePhiSet.empty() indicates that the indvars pass has not being triggered (No PHIs to be rewritten); therefore no changes should be made. That was raised on early feedback by @nikic. And as we know that the loop can be deleted, we just collect the information and pass it to the loop-deletion pass.

Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I still think the whole design here makes little sense. I think this case should either just be Won't Fix, or LoopDeletion needs to compute the exit value, rather than relying on some complex interaction with IndVars (which is not the only user of LoopDeletion!)

Copy link
Member Author

Choose a reason for hiding this comment

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

@nikic Patch already reverted.

If I understand correctly, your main concern is the interaction between IndVars and LoopDeletion passes.

What I would suggest, is to divide the patch into 2 independent ones:

  1. If the IndVars pass is triggered add the missing debug values.
  2. LoopDeletion to compute the exit value.

With no interaction between those 2 passes.

Your feedback would be highly valuable.

}
}
}
}

// 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 %[[SSA_INDEX_0:.+]], metadata ![[DBG:[0-9]+]], {{.*}}

; CHECK: for.extra:
; CHECK: %[[SSA_CALL_0:.+]] = call noundef i32 @"?nop@@YAHH@Z"(i32 noundef %[[SSA_INDEX_0]]), {{.*}}
; CHECK: br i1 %[[SSA_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 %[[SSA_VAR_1:.+]], metadata ![[VAR:[0-9]+]], {{.*}}
; CHECK: br label %for.end, {{.*}}

; CHECK: if.else:
; CHECK: call void @llvm.dbg.value(metadata i32 %[[SSA_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