Add debug location to strlen in LoopIdiomRecognize pass #140164
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Amy Huang (amykhuang) ChangesWe should pass down the debug location to the generated strlen call because LLVM maintains Full diff: https://github.com/llvm/llvm-project/pull/140164.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 8f5d1ecba982d..2b94686ae7ba5 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -1778,6 +1778,9 @@ bool LoopIdiomRecognize::recognizeAndInsertStrLen() {
}
assert(StrLenFunc && "Failed to emit strlen function.");
+ // Set debug location to the start of the loop.
+ cast<Instruction>(StrLenFunc)->setDebugLoc(CurLoop->getStartLoc());
+
const SCEV *StrlenEv = SE->getSCEV(StrLenFunc);
SmallVector<PHINode *, 4> Cleanup;
for (PHINode &PN : LoopExitBB->phis()) {
diff --git a/llvm/test/Transforms/LoopIdiom/strlen.ll b/llvm/test/Transforms/LoopIdiom/strlen.ll
index 3b9d33ebdc74e..3d53b10be3e9b 100644
--- a/llvm/test/Transforms/LoopIdiom/strlen.ll
+++ b/llvm/test/Transforms/LoopIdiom/strlen.ll
@@ -612,3 +612,51 @@ while.end:
ret i64 %sub.ptr.sub
}
+define i64 @valid_basic_strlen_with_dbg(ptr %str) {
+; CHECK-LABEL: define i64 @valid_basic_strlen_with_dbg(
+; CHECK-SAME: ptr [[STR:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: [[STRLEN:%.*]] = call i64 @strlen(ptr [[STR]]), !dbg !{{[0-9]+}}
+; CHECK-NEXT: [[SCEVGEP:%.*]] = getelementptr i8, ptr [[STR]], i64 [[STRLEN]]
+; CHECK-NEXT: br label %[[WHILE_COND:.*]]
+; CHECK: [[WHILE_COND]]:
+; CHECK-NEXT: [[STR_ADDR_0:%.*]] = phi ptr [ [[STR]], %[[ENTRY]] ], [ [[INCDEC_PTR:%.*]], %[[WHILE_COND]] ]
+; CHECK-NEXT: [[TMP0:%.*]] = load i8, ptr [[STR_ADDR_0]], align 1
+; CHECK-NEXT: [[CMP_NOT:%.*]] = icmp eq i8 [[TMP0]], 0
+; CHECK-NEXT: [[INCDEC_PTR]] = getelementptr i8, ptr [[STR_ADDR_0]], i64 1
+; CHECK-NEXT: br i1 true, label %[[WHILE_END:.*]], label %[[WHILE_COND]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: [[SUB_PTR_LHS_CAST:%.*]] = ptrtoint ptr [[SCEVGEP]] to i64
+; CHECK-NEXT: [[SUB_PTR_RHS_CAST:%.*]] = ptrtoint ptr [[STR]] to i64
+; CHECK-NEXT: [[SUB_PTR_SUB:%.*]] = sub i64 [[SUB_PTR_LHS_CAST]], [[SUB_PTR_RHS_CAST]]
+; CHECK-NEXT: ret i64 [[SUB_PTR_SUB]]
+;
+entry:
+ br label %while.cond
+
+while.cond:
+ %str.addr.0 = phi ptr [ %str, %entry ], [ %incdec.ptr, %while.cond ]
+ %0 = load i8, ptr %str.addr.0, align 1, !dbg !6
+ %cmp.not = icmp eq i8 %0, 0, !dbg !6
+ %incdec.ptr = getelementptr i8, ptr %str.addr.0, i64 1
+ br i1 %cmp.not, label %while.end, label %while.cond, !dbg !6
+
+while.end:
+ %sub.ptr.lhs.cast = ptrtoint ptr %str.addr.0 to i64
+ %sub.ptr.rhs.cast = ptrtoint ptr %str to i64
+ %sub.ptr.sub = sub i64 %sub.ptr.lhs.cast, %sub.ptr.rhs.cast
+ ret i64 %sub.ptr.sub
+}
+
+
+!llvm.module.flags = !{!7}
+!llvm.dbg.cu = !{!2}
+
+!0 = distinct !DISubprogram(name: "foo", line: 2, isLocal: false, isDefinition: true, virtualIndex: 6, flags: DIFlagPrototyped, isOptimized: false, unit: !2, file: !1, scope: !1, type: !3)
+!1 = !DIFile(filename: "strlen.c", directory: "/tmp")
+!2 = distinct !DICompileUnit(language: DW_LANG_C99, producer: "clang version 2.9 (trunk 127165:127174)", isOptimized: true, emissionKind: FullDebug, file: !1, enums: !4, retainedTypes: !4)
+!3 = !DISubroutineType(types: !4)
+!4 = !{}
+!5 = distinct !DILexicalBlock(line: 2, column: 21, file: !1, scope: !0)
+!6 = !DILocation(line: 3, column: 3, scope: !5)
+!7 = !{i32 1, !"Debug Info Version", i32 3}
|
| @@ -1778,6 +1778,9 @@ bool LoopIdiomRecognize::recognizeAndInsertStrLen() { | ||
| } | ||
| assert(StrLenFunc && "Failed to emit strlen function."); | ||
|
|
||
| // Set debug location to the start of the loop. | ||
| cast<Instruction>(StrLenFunc)->setDebugLoc(CurLoop->getStartLoc()); |
There was a problem hiding this comment.
Could the static type of StrLenFunc be updated to Instruction* so the cast isn't needed here?
Or, alternatively, perhaps it'd be better to set the debug location on the irbuilder, which will then be used for the instructions?
There was a problem hiding this comment.
Changed to set the debug location on the IRbuilder.
| ; CHECK-LABEL: define i64 @valid_basic_strlen_with_dbg( | ||
| ; CHECK-SAME: ptr [[STR:%.*]]) { | ||
| ; CHECK-NEXT: [[ENTRY:.*]]: | ||
| ; CHECK-NEXT: [[STRLEN:%.*]] = call i64 @strlen(ptr [[STR]]), !dbg !{{[0-9]+}} |
There was a problem hiding this comment.
It'd be good to test that the desired debug location was applied to the resulting strlen - might be this needs to be a test without autogenerated checks to do that.
(& what're all the other instructions here - are they instructions part of the transformation, but not part of the resulting strlen? Do any of those end up with the same location as the strlen and are they correct?)
There was a problem hiding this comment.
Whoops, yeah, I put in the general match because opt was reordering the debug info metadata, but I just reordered it to the "correct" order.
Not sure about the other instructions; they look unchanged so I assume the debug info would remain unchangd as well. Looks like in this particular test case only the compare instruction got replaced with a strlen call. will add the code author too, just realized the change is fairly recent.
| ; CHECK-LABEL: define i64 @valid_basic_strlen_with_dbg( | ||
| ; CHECK-SAME: ptr [[STR:%.*]]) { | ||
| ; CHECK-NEXT: [[ENTRY:.*]]: | ||
| ; CHECK-NEXT: [[STRLEN:%.*]] = call i64 @strlen(ptr [[STR]]), !dbg !4 |
There was a problem hiding this comment.
The metadata numbering might not remain the same across the optimization, I think? - so might be worth matching the !dbg metadata number, then matching the definition of that metadata node/its contents.
There was a problem hiding this comment.
Oh true, I did not think of matching the contents of the debug metadata. fixed
We should pass down the debug location to the generated strlen call because LLVM maintains
that calls to inlinable functions must have debug info.