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

[indvars] Missing variables at Og: #69920

Merged
merged 9 commits into from
Apr 8, 2024

Conversation

CarlosAlbertoEnciso
Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso commented Oct 23, 2023

https://bugs.llvm.org/show_bug.cgi?id=51735
#51077

In the given test case:

4 ...
5 void bar() {
6   int End = 777;
7   int Index = 27;
8   char Var = 1;
9   for (; Index < End; ++Index)
10     ;
11   nop(Index);
12 }
13 ...

Missing local variable Index after loop Induction Variable Elimination.
When adding a breakpoint at line 11, LLDB does not have information
on the variable. But it has info on Var and End.

@llvmbot
Copy link
Member

llvmbot commented Oct 23, 2023

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Carlos Alberto Enciso (CarlosAlbertoEnciso)

Changes

https://bugs.llvm.org/show_bug.cgi?id=51735
#51077

In the given test case:

4 ...
5 void bar() {
6   int End = 777;
7   int Index = 27;
8   char Var = 1;
9   for (; Index &lt; End; ++Index)
10     ;
11   nop(Index);
12 }
13 ...

Missing local variable Index after loop Induction Variable Elimination. When adding a breakpoint at line 11, LLDB does not have information on the variable. But it has info on Var and End.


Full diff: https://github.com/llvm/llvm-project/pull/69920.diff

3 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/IndVarSimplify.cpp (+25)
  • (added) llvm/test/Transforms/LoopSimplify/pr51735-1.ll (+115)
  • (added) llvm/test/Transforms/LoopSimplify/pr51735.ll (+106)
diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
index 41c4d6236173472..0e20b5e62040374 100644
--- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -46,6 +46,7 @@
 #include "llvm/IR/ConstantRange.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
+#include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
@@ -1931,6 +1932,30 @@ bool IndVarSimplify::run(Loop *L) {
     }
   }
 
+  // The loop exit values have been updated; insert the debug location
+  // for the induction variable with its final value.
+  if (PHINode *IndVar = L->getInductionVariable(*SE)) {
+    const SCEV *IndVarSCEV = SE->getSCEVAtScope(IndVar, L->getParentLoop());
+    if (IndVarSCEV->getSCEVType() == SCEVTypes::scConstant) {
+      Value *FinalIVValue = cast<SCEVConstant>(IndVarSCEV)->getValue();
+      SmallVector<DbgVariableIntrinsic *> DbgUsers;
+      SmallVector<DbgVariableIntrinsic *> DbgUsersCloned;
+      findDbgUsers(DbgUsers, IndVar);
+      for (auto &DebugUser : DbgUsers) {
+        auto *Cloned = cast<DbgVariableIntrinsic>(DebugUser->clone());
+        Cloned->replaceVariableLocationOp(static_cast<unsigned>(0),
+                                          FinalIVValue);
+        DbgUsersCloned.push_back(Cloned);
+      }
+
+      SmallVector<BasicBlock *> ExitBlocks;
+      L->getExitBlocks(ExitBlocks);
+      for (BasicBlock *Exit : ExitBlocks)
+        for (auto &DebugUser : DbgUsersCloned)
+          DebugUser->insertBefore(Exit->getFirstNonPHI());
+    }
+  }
+
   // Eliminate redundant IV cycles.
   NumElimIV += Rewriter.replaceCongruentIVs(L, DT, DeadInsts, TTI);
 
diff --git a/llvm/test/Transforms/LoopSimplify/pr51735-1.ll b/llvm/test/Transforms/LoopSimplify/pr51735-1.ll
new file mode 100644
index 000000000000000..de9a74522095659
--- /dev/null
+++ b/llvm/test/Transforms/LoopSimplify/pr51735-1.ll
@@ -0,0 +1,115 @@
+; RUN: opt -passes=indvars -S -o - < %s | FileCheck %s
+
+; Missing local variable 'Index' after loop 'Induction Variable Elimination'.
+; When adding a breakpoint at line 11, LLDB does not have information on
+; the variable. But it has info on 'Var' and 'End'.
+
+;  1	__attribute__((optnone)) int nop(int Param) {
+;  2	  return 0;
+;  3	}
+;  4
+;  5	void bar() {
+;  6    int End = 777;
+;  7	  int Index = 27;
+;  8	  char Var = 1;
+;  9	  for (; Index < End; ++Index)
+; 10	    ;
+; 11	  nop(Index);
+; 12	}
+; 13
+; 14	int main () {
+; 15	  bar();
+; 16	}
+
+; CHECK: for.cond: {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 poison, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 poison, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   br i1 false, label %for.cond, label %for.end, {{.*}}
+; CHECK: for.end: {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 777, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   %call = tail call noundef i32 @_Z3nopi(i32 noundef 777), {{.*}}
+; CHECK-DAG: ![[DBG]] = !DILocalVariable(name: "Index"{{.*}})
+
+define dso_local noundef i32 @_Z3nopi(i32 noundef %Param) local_unnamed_addr #0 !dbg !10 {
+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 !15, metadata !DIExpression()), !dbg !16
+  ret i32 0, !dbg !17
+}
+
+define dso_local void @_Z3barv() local_unnamed_addr #2 !dbg !18 {
+entry:
+  call void @llvm.dbg.value(metadata i32 777, metadata !21, metadata !DIExpression()), !dbg !22
+  call void @llvm.dbg.value(metadata i32 27, metadata !23, metadata !DIExpression()), !dbg !22
+  call void @llvm.dbg.value(metadata i32 1, metadata !24, metadata !DIExpression()), !dbg !22
+  br label %for.cond, !dbg !25
+
+for.cond:                                         ; preds = %for.cond, %entry
+  %Index.0 = phi i32 [ 27, %entry ], [ %inc, %for.cond ], !dbg !22
+  call void @llvm.dbg.value(metadata i32 %Index.0, metadata !23, metadata !DIExpression()), !dbg !22
+  %cmp = icmp ult i32 %Index.0, 777, !dbg !26
+  %inc = add nuw nsw i32 %Index.0, 1, !dbg !29
+  call void @llvm.dbg.value(metadata i32 %inc, metadata !23, metadata !DIExpression()), !dbg !22
+  br i1 %cmp, label %for.cond, label %for.end, !dbg !30, !llvm.loop !31
+
+for.end:                                          ; preds = %for.cond
+  %Index.0.lcssa = phi i32 [ %Index.0, %for.cond ], !dbg !22
+  %call = tail call noundef i32 @_Z3nopi(i32 noundef %Index.0.lcssa), !dbg !34
+  ret void, !dbg !35
+}
+
+define dso_local noundef i32 @main() local_unnamed_addr #3 !dbg !36 {
+entry:
+  call void @_Z3barv(), !dbg !39
+  ret i32 0, !dbg !40
+}
+
+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 version 18.0.0 (https://github.com/llvm/llvm-project.git 18c2eb2bf02bd7666523aa566e45d62053b7db80)", 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 = distinct !DISubprogram(name: "nop", linkageName: "_Z3nopi", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !14)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, !13}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !{}
+!15 = !DILocalVariable(name: "Param", arg: 1, scope: !10, file: !1, line: 1, type: !13)
+!16 = !DILocation(line: 1, column: 38, scope: !10)
+!17 = !DILocation(line: 2, column: 3, scope: !10)
+!18 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !1, file: !1, line: 5, type: !19, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !14)
+!19 = !DISubroutineType(types: !20)
+!20 = !{null}
+!21 = !DILocalVariable(name: "End", scope: !18, file: !1, line: 6, type: !13)
+!22 = !DILocation(line: 0, scope: !18)
+!23 = !DILocalVariable(name: "Index", scope: !18, file: !1, line: 7, type: !13)
+!24 = !DILocalVariable(name: "Var", scope: !18, file: !1, line: 8, type: !13)
+!25 = !DILocation(line: 9, column: 3, scope: !18)
+!26 = !DILocation(line: 9, column: 16, scope: !27)
+!27 = distinct !DILexicalBlock(scope: !28, file: !1, line: 9, column: 3)
+!28 = distinct !DILexicalBlock(scope: !18, file: !1, line: 9, column: 3)
+!29 = !DILocation(line: 9, column: 23, scope: !27)
+!30 = !DILocation(line: 9, column: 3, scope: !28)
+!31 = distinct !{!31, !30, !32, !33}
+!32 = !DILocation(line: 10, column: 5, scope: !28)
+!33 = !{!"llvm.loop.mustprogress"}
+!34 = !DILocation(line: 11, column: 3, scope: !18)
+!35 = !DILocation(line: 12, column: 1, scope: !18)
+!36 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 14, type: !37, scopeLine: 14, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!37 = !DISubroutineType(types: !38)
+!38 = !{!13}
+!39 = !DILocation(line: 15, column: 3, scope: !36)
+!40 = !DILocation(line: 16, column: 1, scope: !36)
diff --git a/llvm/test/Transforms/LoopSimplify/pr51735.ll b/llvm/test/Transforms/LoopSimplify/pr51735.ll
new file mode 100644
index 000000000000000..b9bdc8ca1b6fb09
--- /dev/null
+++ b/llvm/test/Transforms/LoopSimplify/pr51735.ll
@@ -0,0 +1,106 @@
+; RUN: opt -passes=indvars -S -o - < %s | FileCheck %s
+
+; Missing local variable 'Index' after loop 'Induction Variable Elimination'.
+; When adding a breakpoint at line 11, LLDB does not have information on
+; the variable. But it has info on 'Var' and 'End'.
+
+;  1	__attribute__((optnone)) int nop() {
+;  2	  return 0;
+;  3	}
+;  4
+;  5	void bar() {
+;  6    int End = 777;
+;  7	  int Index = 27;
+;  8	  char Var = 1;
+;  9	  for (; Index < End; ++Index)
+; 10	    ;
+; 11	  nop();
+; 12	}
+; 13
+; 14	int main () {
+; 15	  bar();
+; 16	}
+
+; CHECK: for.cond: {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 poison, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 poison, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   br i1 false, label %for.cond, label %for.end, {{.*}}
+; CHECK: for.end: {{.*}}
+; CHECK:   call void @llvm.dbg.value(metadata i32 777, metadata ![[DBG:[0-9]+]], {{.*}}
+; CHECK:   %call = tail call noundef i32 @_Z3nopv(), {{.*}}
+; CHECK-DAG: ![[DBG]] = !DILocalVariable(name: "Index"{{.*}})
+
+define dso_local noundef i32 @_Z3nopv() local_unnamed_addr #0 !dbg !10 {
+entry:
+  ret i32 0, !dbg !14
+}
+
+define dso_local void @_Z3barv() local_unnamed_addr #1 !dbg !15 {
+entry:
+  call void @llvm.dbg.value(metadata i32 777, metadata !19, metadata !DIExpression()), !dbg !20
+  call void @llvm.dbg.value(metadata i32 27, metadata !21, metadata !DIExpression()), !dbg !20
+  call void @llvm.dbg.value(metadata i32 1, metadata !22, metadata !DIExpression()), !dbg !20
+  br label %for.cond, !dbg !23
+
+for.cond:                                         ; preds = %for.cond, %entry
+  %Index.0 = phi i32 [ 27, %entry ], [ %inc, %for.cond ], !dbg !20
+  call void @llvm.dbg.value(metadata i32 %Index.0, metadata !21, metadata !DIExpression()), !dbg !20
+  %cmp = icmp ult i32 %Index.0, 777, !dbg !24
+  %inc = add nuw nsw i32 %Index.0, 1, !dbg !27
+  call void @llvm.dbg.value(metadata i32 %inc, metadata !21, metadata !DIExpression()), !dbg !20
+  br i1 %cmp, label %for.cond, label %for.end, !dbg !28, !llvm.loop !29
+
+for.end:                                          ; preds = %for.cond
+  %call = tail call noundef i32 @_Z3nopv(), !dbg !32
+  ret void, !dbg !33
+}
+
+define dso_local noundef i32 @main() local_unnamed_addr #2 !dbg !34 {
+entry:
+  call void @_Z3barv(), !dbg !35
+  ret i32 0, !dbg !36
+}
+
+declare void @llvm.dbg.value(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 version 18.0.0 (https://github.com/llvm/llvm-project.git 18c2eb2bf02bd7666523aa566e45d62053b7db80)", 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 = distinct !DISubprogram(name: "nop", linkageName: "_Z3nopv", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!14 = !DILocation(line: 2, column: 3, scope: !10)
+!15 = distinct !DISubprogram(name: "bar", linkageName: "_Z3barv", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !18)
+!16 = !DISubroutineType(types: !17)
+!17 = !{null}
+!18 = !{}
+!19 = !DILocalVariable(name: "End", scope: !15, file: !1, line: 6, type: !13)
+!20 = !DILocation(line: 0, scope: !15)
+!21 = !DILocalVariable(name: "Index", scope: !15, file: !1, line: 7, type: !13)
+!22 = !DILocalVariable(name: "Var", scope: !15, file: !1, line: 8, type: !13)
+!23 = !DILocation(line: 9, column: 3, scope: !15)
+!24 = !DILocation(line: 9, column: 16, scope: !25)
+!25 = distinct !DILexicalBlock(scope: !26, file: !1, line: 9, column: 3)
+!26 = distinct !DILexicalBlock(scope: !15, file: !1, line: 9, column: 3)
+!27 = !DILocation(line: 9, column: 23, scope: !25)
+!28 = !DILocation(line: 9, column: 3, scope: !26)
+!29 = distinct !{!29, !28, !30, !31}
+!30 = !DILocation(line: 10, column: 5, scope: !26)
+!31 = !{!"llvm.loop.mustprogress"}
+!32 = !DILocation(line: 11, column: 3, scope: !15)
+!33 = !DILocation(line: 12, column: 1, scope: !15)
+!34 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 14, type: !11, scopeLine: 14, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0)
+!35 = !DILocation(line: 15, column: 3, scope: !34)
+!36 = !DILocation(line: 16, column: 1, scope: !34)

llvm/lib/Transforms/Scalar/IndVarSimplify.cpp Outdated Show resolved Hide resolved
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp Outdated Show resolved Hide resolved
llvm/test/Transforms/LoopSimplify/pr51735-1.ll Outdated Show resolved Hide resolved
llvm/test/Transforms/LoopSimplify/pr51735-1.ll Outdated Show resolved Hide resolved
@CarlosAlbertoEnciso
Copy link
Member Author

@nikic Thanks for your feedback.
I uploaded a new patch addressing:

  • Moved the new logic to rewriteLoopExitValues.
  • Removed from the test cases, the unrelated functions: nop and main.
  • Use isa<SCEVConstant>.


// The loop exit values have been updated; insert the debug location
// for the induction variable with its final value.
if (PHINode *IndVar = L->getInductionVariable(*SE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still the wrong place to do this. You are updating just a single "induction variable" here, while the code can rewrite multiple, and can also use different exit values for different blocks. This adjustment should be above in the // Transformation. loop, which has all the induction phis and their replacement values.

if (PHINode *IndVar = L->getInductionVariable(*SE)) {
const SCEV *IndVarSCEV = SE->getSCEVAtScope(IndVar, L->getParentLoop());
if (isa<SCEVConstant>(IndVarSCEV)) {
Value *FinalIVValue = cast<SCEVConstant>(IndVarSCEV)->getValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you should not use an isa<> test followed by a cast<>, for that use the dyn_cast<> operator.

https://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

I think this would be easier to follow with an early exit on the result of the dyn cast: https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

findDbgUsers(DbgUsers, IndVar);
for (auto &DebugUser : DbgUsers) {
auto *Cloned = cast<DbgVariableIntrinsic>(DebugUser->clone());
Cloned->replaceVariableLocationOp(static_cast<unsigned>(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

0u

SmallVector<DbgVariableIntrinsic *> DbgUsers;
SmallVector<DbgVariableIntrinsic *> DbgUsersCloned;
findDbgUsers(DbgUsers, IndVar);
for (auto &DebugUser : DbgUsers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need a reference to a pointer here (we are not modifying the pointer), we can use auto * to express this intent

SmallVector<BasicBlock *> ExitBlocks;
L->getExitBlocks(ExitBlocks);
for (BasicBlock *Exit : ExitBlocks)
for (auto &DebugUser : DbgUsersCloned)
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly here auto *

; 15 bar();
; 16 }

; CHECK: for.cond: {{.*}}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this {{.*}}? (and in the other BB labels)

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

Choose a reason for hiding this comment

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

if I understand this test correctly, all of these dbg.values are for the variable Index, right? I think it's important that we label the first occurrence of it and check that all three of these intrinsics are referring to it (right now we allow any metadata there)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that we are redefining what ![[DBG]] is in each of these CHECKs


define dso_local void @_Z3barv() local_unnamed_addr #2 !dbg !18 {
entry:
call void @llvm.dbg.value(metadata i32 777, metadata !21, metadata !DIExpression()), !dbg !22
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this and the third intrinsic relevant for the test? (i.e. the intrinsics that don't target !23 = Index

; CHECK: call void @llvm.dbg.value(metadata i32 777, metadata ![[DBG:[0-9]+]], {{.*}}
; CHECK-DAG: ![[DBG]] = !DILocalVariable(name: "Index"{{.*}})

define dso_local void @_Z3barv() local_unnamed_addr #1 !dbg !15 {
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 elaborate on how these tests differ from each other? I couldn't tell by comparing the two

Copy link
Member

Choose a reason for hiding this comment

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

(IIRC, the difference is that there are multiple exit blocks, to stimulate more of the paths through the code-changes).

@CarlosAlbertoEnciso
Copy link
Member Author

I am sorry for my delay in addressing the comments from the reviewers, but I was on PTO.
The reworked patch addresses the issues and includes 2 test cases:

  • Multiple exit blocks
  • Single exit block.

// get updated, which is fine as that is the existing behaviour.
if (DebugUser->hasArgList())
continue;
for (BasicBlock *Exit : ExitBlocks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The ExitValue is specific to one exit block. Why are you updating all of them here?

Copy link
Member Author

@CarlosAlbertoEnciso CarlosAlbertoEnciso Jan 9, 2024

Choose a reason for hiding this comment

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

Reworked the code to take into account that point. Reworked in new patch.

@@ -1482,6 +1513,21 @@ 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 if the PN is a induction variable.
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 this code needed in addition to the addDebugValuesToLoopVariable() below?

Copy link
Member Author

Choose a reason for hiding this comment

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

That code creates a debug value in the exit block if any of the incoming values for the PHI Node being replaced is the induction variable. Reworked in new patch.

@CarlosAlbertoEnciso CarlosAlbertoEnciso force-pushed the pr-51077 branch 2 times, most recently from 74d48ef to 88f7d8c Compare January 9, 2024 14:17
@CarlosAlbertoEnciso
Copy link
Member Author

Uploaded a new patch that addresses @nikic feedback.

Copy link

github-actions bot commented Jan 9, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@CarlosAlbertoEnciso
Copy link
Member Author

Uploaded patch fixing the C/C++ code formatter, clang-format found issues.

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

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

For the change adding the "ExitBlock" field to the RewritePhi struct -- as far as understand it, the PHI being rewritten is always going to be in that exit block. Perhaps we could not store it in the struct and instead recover the exit block by calling getParent() on the PHI when necessary, instead? (This avoids two storing two pieces of information and having to think about whether they're in sync or not).

PN->replaceAllUsesWith(ExitVal);
PN->eraseFromParent();
}
}

// If there are no PHIs to be rewritten then there are no loop live-out
// values, try to rewrite variables corresponding to the induction variable
Copy link
Member

Choose a reason for hiding this comment

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

I reckon you want to say "rewrite debug variables" or similar, just to signal to the reader that these aren't for SSA variables which might have a functional effect on the program.

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.

// will completely forget that this loop happened.
if (RewritePhiSet.empty()) {
// The loop exit value has been updated; insert the debug location
// for the given the induction variable with its final value.
Copy link
Member

Choose a reason for hiding this comment

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

"for the given induction..."

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.

@CarlosAlbertoEnciso
Copy link
Member Author

For the change adding the "ExitBlock" field to the RewritePhi struct -- as far as understand it, the PHI being rewritten is always going to be in that exit block. Perhaps we could not store it in the struct and instead recover the exit block by calling getParent() on the PHI when necessary, instead? (This avoids two storing two pieces of information and having to think about whether they're in sync or not).

Good point. Removed the added field.

https://bugs.llvm.org/show_bug.cgi?id=51735
llvm#51077

After internal discussion with @jmorse, it was decided
to split the work between the 'indvars' and the
'loop-deletion' passes.

1) passes="loop(indvars)"
- 'indvars' transformation is fired:
  the 'rewriteLoopExitValues' will rewrite the collected
  PNs with the exit values.
- 'indvars' transformation is not fired:
  If the loop can be deleted, we preserve the induction
  variable information to be used by 'loop-deletion' if
  that pass will be executed.

2) passes="loop(indvars,loop-deletion)"
  If the loop is deleted in 'deleteDeadLoop' and there
  is a valid exit block, we use any collected values
  by 'indvars' to updated the exit values.

Added extra tests to cover the following cases:
  ...
  char Var = 1;
  for (; Index < End; ++Index)
    if (Index == 666)
      ++Var;
  ...

and
  ...
  char Var = 1;
  for (; Index < End; ++Index)
    if (Index == 666)
      Var = 555;
  ...

Modified but otherwise unused variable 'Var' in a loop
that gets deleted.
Copy link
Contributor

@SLTozer SLTozer left a comment

Choose a reason for hiding this comment

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

Largely looks good, some comments inline.

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.

; PRE-CHECK: br label %for.cond

; PRE-CHECK: for.end:
; PRE-CHECK-NOT: call void @llvm.dbg.value
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be skipped since there's an implicit-check-not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.


; Only the 'indvars' pass is executed.
; PRE-CHECK: for.cond:
; PRE-CHECK: call void @llvm.dbg.value(metadata i32 %Var.0, metadata ![[DBG]], {{.*}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This and a lot of the other check lines may fail in a build without assertions enabled, as the SSA values won't necessarily be printed with the same names (they may become numbered values, e.g. %5, %6, etc). Usually the way to test these would then be to use capture variables for them, e.g. i32 %[[SSA_VAR0:.+]],. This gets tedious if you have to use it for every variable, so I'd recommend removing the unrelated CHECK lines, which would be most of the lines that don't need to be tested for and don't produce a value that we want to check; or instead of removing them, you could cut out actual SSA values, i.e. you could just test for = icmp eq.

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. Changed to use captured variables.

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.

https://bugs.llvm.org/show_bug.cgi?id=51735
llvm#51077

Address @SLTozer comments.
- Use captured variables for SSA values.
- Use new insert logic calls.
- Remove not required 'CHECK-NOT'.
@CarlosAlbertoEnciso
Copy link
Member Author

@SLTozer Thanks for your comments. Updated the patch.

@SLTozer
Copy link
Contributor

SLTozer commented Mar 18, 2024

One more part - you've captured the SSA values that are needed for the test, but for the SSA values you're not using you should still remove the actual names so that if the identifiers change (which can happen when building with assertions enabled disabled I think) the test doesn't fail. Since you don't need to use the captured names, you can just use {{.+}}, or else you can just omit some of the lines in whole or in part.

https://bugs.llvm.org/show_bug.cgi?id=51735
llvm#51077

Address @SLTozer comments.
- Don't use any names for not used SSA values.
@CarlosAlbertoEnciso
Copy link
Member Author

@SLTozer Thanks for the suggested changes when dealing with the not used SSA values.

@CarlosAlbertoEnciso
Copy link
Member Author

Any additional comments or suggestions? Thanks.

@CarlosAlbertoEnciso CarlosAlbertoEnciso merged commit 739fa1c into llvm:main Apr 8, 2024
4 checks passed
@CarlosAlbertoEnciso
Copy link
Member Author

Thanks to all reviewers for their comments.

@nikic
Copy link
Contributor

nikic commented Apr 8, 2024

This introduces a layering violation. GenericLoopInfo in Support is not allowed to depend on IR. That's the point of the GenericXYZ implementations, they are IR-agnostic.

Edit: Reverted in 91189af.

// 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.

nikic added a commit that referenced this pull request Apr 8, 2024
This reverts commit 739fa1c.

This introduces a layering violation by using IR in Support headers.
@CarlosAlbertoEnciso
Copy link
Member Author

CarlosAlbertoEnciso commented Apr 10, 2024

Reopen as #88270 to address @nikic comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants