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

[DebugInfo][LoopIdiomRecognize] Fix #82582: Wrong debug location update in processLoopStoreOfLoopLoad #82608

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Apochens
Copy link
Contributor

This PR partially fixes #82582 , correcting the debug location of the NewCall variable in the function processLoopStoreOfLoopLoad (instead of preversing, we drop its debug location). I also add the correponding regression test case constructed from the C source code mentioned in the above issue. Here, I give the original C source code and compilation command of the test case for convinience:

void fun(int *__restrict__ a, int *__restrict__ b) {
    for (long i = 2047; i >= 0; i--) {
        a[i] = b[i];
    }
}

int main() {
    int a[2048], b[2048];
    fun(a, b);
    return 0;
}
$ clang -S -emit-llvm -Xclang -disable-O0-optnone test.c -g -o test.ll
$ opt -S -passes=mem2reg,loop-rotate,loop-simplifycfg test.ll -o test-idiom.ll

In addition to the above modificaitons, the regressiong test case memcpy-debugify-remarks.ll needs to be modified. In the original test, it assumes that the NewCall has a debug location, so there are checks on the debug location. Now, since we drop the debug location, we need to remove these checks.

@llvmbot
Copy link
Collaborator

llvmbot commented Feb 22, 2024

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Shan Huang (Apochens)

Changes

This PR partially fixes #82582 , correcting the debug location of the NewCall variable in the function processLoopStoreOfLoopLoad (instead of preversing, we drop its debug location). I also add the correponding regression test case constructed from the C source code mentioned in the above issue. Here, I give the original C source code and compilation command of the test case for convinience:

void fun(int *__restrict__ a, int *__restrict__ b) {
    for (long i = 2047; i >= 0; i--) {
        a[i] = b[i];
    }
}

int main() {
    int a[2048], b[2048];
    fun(a, b);
    return 0;
}
$ clang -S -emit-llvm -Xclang -disable-O0-optnone test.c -g -o test.ll
$ opt -S -passes=mem2reg,loop-rotate,loop-simplifycfg test.ll -o test-idiom.ll

In addition to the above modificaitons, the regressiong test case memcpy-debugify-remarks.ll needs to be modified. In the original test, it assumes that the NewCall has a debug location, so there are checks on the debug location. Now, since we drop the debug location, we need to remove these checks.


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

3 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp (+2-1)
  • (modified) llvm/test/Transforms/LoopIdiom/memcpy-debugify-remarks.ll (+2-3)
  • (added) llvm/test/Transforms/LoopIdiom/processloopstoreforloopload-drop-debugloc.ll (+103)
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 3721564890ddb4..5359b580de2062 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -1418,7 +1418,8 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(
         StoreBasePtr, *StoreAlign, LoadBasePtr, *LoadAlign, NumBytes, StoreSize,
         AATags.TBAA, AATags.TBAAStruct, AATags.Scope, AATags.NoAlias);
   }
-  NewCall->setDebugLoc(TheStore->getDebugLoc());
+  
+  NewCall->dropLocation();
 
   if (MSSAU) {
     MemoryAccess *NewMemAcc = MSSAU->createMemoryAccessInBB(
diff --git a/llvm/test/Transforms/LoopIdiom/memcpy-debugify-remarks.ll b/llvm/test/Transforms/LoopIdiom/memcpy-debugify-remarks.ll
index 3a48b178123c47..49f3bed55b9ff6 100644
--- a/llvm/test/Transforms/LoopIdiom/memcpy-debugify-remarks.ll
+++ b/llvm/test/Transforms/LoopIdiom/memcpy-debugify-remarks.ll
@@ -10,12 +10,11 @@ target triple = "x86_64-unknown-linux-gnu"
 
 ; Check that everything still works when debuginfo is present, and that it is reasonably propagated.
 
-; CHECK: remark: <stdin>:6:1: Formed a call to llvm.memcpy.p0.p0.i64() intrinsic from load and store instruction in test6_dest_align function{{$}}
+; CHECK: remark: <unknown>:0:0: Formed a call to llvm.memcpy.p0.p0.i64() intrinsic from load and store instruction in test6_dest_align function{{$}}
 
 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            loop-idiom
 ; YAML-NEXT: Name:            ProcessLoopStoreOfLoopLoad
-; YAML-NEXT: DebugLoc:        { File: '<stdin>', Line: 6, Column: 1 }
 ; YAML-NEXT: Function:        test6_dest_align
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - String:          'Formed a call to '
@@ -34,7 +33,7 @@ define void @test6_dest_align(ptr noalias align 1 %Base, ptr noalias align 4 %De
 ; CHECK-LABEL: @test6_dest_align(
 ; CHECK-NEXT:  bb.nph:
 ; CHECK-NEXT:    [[TMP0:%.*]] = shl nuw i64 [[SIZE:%.*]], 2, !dbg [[DBG18:![0-9]+]]
-; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 4 [[DEST:%.*]], ptr align 1 [[BASE:%.*]], i64 [[TMP0]], i1 false), !dbg [[DBG19:![0-9]+]]
+; CHECK-NEXT:    call void @llvm.memcpy.p0.p0.i64(ptr align 4 [[DEST:%.*]], ptr align 1 [[BASE:%.*]], i64 [[TMP0]], i1 false)
 ; CHECK-NEXT:    br label [[FOR_BODY:%.*]], !dbg [[DBG18]]
 ; CHECK:       for.body:
 ; CHECK-NEXT:    [[INDVAR:%.*]] = phi i64 [ 0, [[BB_NPH:%.*]] ], [ [[INDVAR_NEXT:%.*]], [[FOR_BODY]] ], !dbg [[DBG20:![0-9]+]]
diff --git a/llvm/test/Transforms/LoopIdiom/processloopstoreforloopload-drop-debugloc.ll b/llvm/test/Transforms/LoopIdiom/processloopstoreforloopload-drop-debugloc.ll
new file mode 100644
index 00000000000000..3a15f32388eda9
--- /dev/null
+++ b/llvm/test/Transforms/LoopIdiom/processloopstoreforloopload-drop-debugloc.ll
@@ -0,0 +1,103 @@
+; RUN: opt < %s -passes=loop-idiom -S | FileCheck %s
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local void @fun(ptr noalias noundef %a, ptr noalias noundef %b) #0 !dbg !10 {
+
+; CHECK-LABEL: entry:
+; CHECK-NOT: call void @llvm.memcpy.p0.p0.i64{{.*}}dbg {{![0-9]+}}
+entry:
+  tail call void @llvm.dbg.value(metadata ptr %a, metadata !17, metadata !DIExpression()), !dbg !18
+  tail call void @llvm.dbg.value(metadata ptr %b, metadata !19, metadata !DIExpression()), !dbg !18
+  tail call void @llvm.dbg.value(metadata i64 2047, metadata !20, metadata !DIExpression()), !dbg !23
+  br label %for.body, !dbg !24
+
+for.body:                                         ; preds = %entry, %for.body
+  %i.01 = phi i64 [ 2047, %entry ], [ %dec, %for.body ]
+  tail call void @llvm.dbg.value(metadata i64 %i.01, metadata !20, metadata !DIExpression()), !dbg !23
+  %arrayidx = getelementptr inbounds i32, ptr %b, i64 %i.01, !dbg !25
+  %0 = load i32, ptr %arrayidx, align 4, !dbg !25
+  %arrayidx1 = getelementptr inbounds i32, ptr %a, i64 %i.01, !dbg !28
+  store i32 %0, ptr %arrayidx1, align 4, !dbg !29
+  %dec = add nsw i64 %i.01, -1, !dbg !30
+  tail call void @llvm.dbg.value(metadata i64 %dec, metadata !20, metadata !DIExpression()), !dbg !23
+  %cmp = icmp sge i64 %dec, 0, !dbg !31
+  br i1 %cmp, label %for.body, label %for.end, !dbg !24, !llvm.loop !32
+
+for.end:                                          ; preds = %for.body
+  ret void, !dbg !35
+}
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.declare(metadata, metadata, metadata) #1
+
+; Function Attrs: noinline nounwind uwtable
+define dso_local i32 @main() #0 !dbg !36 {
+entry:
+  %a = alloca [2048 x i32], align 16
+  %b = alloca [2048 x i32], align 16
+  tail call void @llvm.dbg.declare(metadata ptr %a, metadata !39, metadata !DIExpression()), !dbg !43
+  tail call void @llvm.dbg.declare(metadata ptr %b, metadata !44, metadata !DIExpression()), !dbg !45
+  %arraydecay = getelementptr inbounds [2048 x i32], ptr %a, i64 0, i64 0, !dbg !46
+  %arraydecay1 = getelementptr inbounds [2048 x i32], ptr %b, i64 0, i64 0, !dbg !47
+  call void @fun(ptr noundef %arraydecay, ptr noundef %arraydecay1), !dbg !48
+  ret i32 0, !dbg !49
+}
+
+; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none)
+declare void @llvm.dbg.value(metadata, metadata, metadata) #1
+
+attributes #0 = { noinline nounwind uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5, !6, !7, !8}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 18.0.0git (https://github.com/llvm/llvm-project.git 7e604485e18d40be6ce6310e4a3e583ca0b7df47)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "1423.c", directory: "/home/linuxbrew/llvm-debug/LoopIdiomRecognize", checksumkind: CSK_MD5, checksum: "50c6f3e45074a3e94d8587b0957b242b")
+!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}
+!10 = distinct !DISubprogram(name: "fun", scope: !1, file: !1, line: 1, type: !11, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !16)
+!11 = !DISubroutineType(types: !12)
+!12 = !{null, !13, !13}
+!13 = !DIDerivedType(tag: DW_TAG_restrict_type, baseType: !14)
+!14 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !15, size: 64)
+!15 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!16 = !{}
+!17 = !DILocalVariable(name: "a", arg: 1, scope: !10, file: !1, line: 1, type: !13)
+!18 = !DILocation(line: 0, scope: !10)
+!19 = !DILocalVariable(name: "b", arg: 2, scope: !10, file: !1, line: 1, type: !13)
+!20 = !DILocalVariable(name: "i", scope: !21, file: !1, line: 2, type: !22)
+!21 = distinct !DILexicalBlock(scope: !10, file: !1, line: 2, column: 5)
+!22 = !DIBasicType(name: "long", size: 64, encoding: DW_ATE_signed)
+!23 = !DILocation(line: 0, scope: !21)
+!24 = !DILocation(line: 2, column: 5, scope: !21)
+!25 = !DILocation(line: 3, column: 16, scope: !26)
+!26 = distinct !DILexicalBlock(scope: !27, file: !1, line: 2, column: 38)
+!27 = distinct !DILexicalBlock(scope: !21, file: !1, line: 2, column: 5)
+!28 = !DILocation(line: 3, column: 9, scope: !26)
+!29 = !DILocation(line: 3, column: 14, scope: !26)
+!30 = !DILocation(line: 2, column: 34, scope: !27)
+!31 = !DILocation(line: 2, column: 27, scope: !27)
+!32 = distinct !{!32, !24, !33, !34}
+!33 = !DILocation(line: 4, column: 5, scope: !21)
+!34 = !{!"llvm.loop.mustprogress"}
+!35 = !DILocation(line: 5, column: 1, scope: !10)
+!36 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 7, type: !37, scopeLine: 7, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !16)
+!37 = !DISubroutineType(types: !38)
+!38 = !{!15}
+!39 = !DILocalVariable(name: "a", scope: !36, file: !1, line: 8, type: !40)
+!40 = !DICompositeType(tag: DW_TAG_array_type, baseType: !15, size: 65536, elements: !41)
+!41 = !{!42}
+!42 = !DISubrange(count: 2048)
+!43 = !DILocation(line: 8, column: 9, scope: !36)
+!44 = !DILocalVariable(name: "b", scope: !36, file: !1, line: 8, type: !40)
+!45 = !DILocation(line: 8, column: 18, scope: !36)
+!46 = !DILocation(line: 9, column: 9, scope: !36)
+!47 = !DILocation(line: 9, column: 12, scope: !36)
+!48 = !DILocation(line: 9, column: 5, scope: !36)
+!49 = !DILocation(line: 10, column: 5, scope: !36)

Copy link

github-actions bot commented Feb 22, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 8bd327d6fed5a4ae99bdbd039f5503700030cf53 d2eda06a963eae024dd20658752f9ed79fa51246 -- llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
View the diff from clang-format here.
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 1543c7db8c..4695d6df1e 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -1418,7 +1418,7 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(
         StoreBasePtr, *StoreAlign, LoadBasePtr, *LoadAlign, NumBytes, StoreSize,
         AATags.TBAA, AATags.TBAAStruct, AATags.Scope, AATags.NoAlias);
   }
-  
+
   // As the `NewCall` is created in the preheader, it is out of the loop.
   // The `TheStore` is in the loop body, so set the debug location of the
   // `TheStore` to the `NewCall` will make debugging confusing.
@@ -1444,8 +1444,7 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(
            << ore::NV("NewFunction", NewCall->getCalledFunction())
            << "() intrinsic from " << ore::NV("Inst", InstRemark)
            << " instruction in " << ore::NV("Function", TheStore->getFunction())
-           << " function"
-           << ore::setExtraArgs()
+           << " function" << ore::setExtraArgs()
            << ore::NV("FromBlock", TheStore->getParent()->getName())
            << ore::NV("ToBlock", Preheader->getName());
   });

@Apochens Apochens changed the title [LoopIdiomRecognize] Fix #82582: Wrong debug location updates in LoopIdiomRecognize [DebugInfo][LoopIdiomRecognize] Fix #82582: Wrong debug location update in processLoopStoreOfLoopLoad Feb 22, 2024
@Apochens
Copy link
Contributor Author

Hi, @nikic ! Could you please review this PR?

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.

Thanks for this, always good to catch these kind of DILocation issues - got a few minor comments on this, otherwise it looks good.

define dso_local void @fun(ptr noalias noundef %a, ptr noalias noundef %b) #0 !dbg !10 {

; CHECK-LABEL: entry:
; CHECK-NOT: call void @llvm.memcpy.p0.p0.i64{{.*}}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.

Might be a good idea to add a positive check that the memcpy instruction does appear - if something changes in the future and we produce a different instruction for this optimization, or this optimization stops happening in this case, this test will continue to pass even if the error reappears.

Comment on lines 49 to 50
attributes #0 = { noinline nounwind uwtable "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #1 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) }
Copy link
Contributor

Choose a reason for hiding this comment

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

For these tests, we prefer to drop the function attributes if they aren't necessary for the test to pass.

Comment on lines 55 to 56
!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 18.0.0git (https://github.com/llvm/llvm-project.git 7e604485e18d40be6ce6310e4a3e583ca0b7df47)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "1423.c", directory: "/home/linuxbrew/llvm-debug/LoopIdiomRecognize", checksumkind: CSK_MD5, checksum: "50c6f3e45074a3e94d8587b0957b242b")
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
!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 18.0.0git (https://github.com/llvm/llvm-project.git 7e604485e18d40be6ce6310e4a3e583ca0b7df47)", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "1423.c", directory: "/home/linuxbrew/llvm-debug/LoopIdiomRecognize", checksumkind: CSK_MD5, checksum: "50c6f3e45074a3e94d8587b0957b242b")
!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 18.0.0", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
!1 = !DIFile(filename: "1423.c", directory: "/home/linuxbrew/llvm-debug/LoopIdiomRecognize")

We can drop the commit hash and checksum info.

@@ -10,12 +10,11 @@ target triple = "x86_64-unknown-linux-gnu"

; Check that everything still works when debuginfo is present, and that it is reasonably propagated.

; CHECK: remark: <stdin>:6:1: Formed a call to llvm.memcpy.p0.p0.i64() intrinsic from load and store instruction in test6_dest_align function{{$}}
; CHECK: remark: <unknown>:0:0: Formed a call to llvm.memcpy.p0.p0.i64() intrinsic from load and store instruction in test6_dest_align function{{$}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping the location for NewCall is the right thing - but it seems to me that the optimization remark would be more useful if we retained the old debug loc, which is still correct for attribution purposes (though unfortunately we can't separate stepping and attribution concerns at the moment); you could change the optimization remark to refer to TheStore's DebugLoc, and we could get a meaningful source location here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've already changed the optimization remark to refer to TheStore's DebugLoc and reverted the change in this check.

@jmorse
Copy link
Member

jmorse commented Feb 23, 2024

This is good defect to be found; unfortunately as Stephen says there's a conflict between stepping and attribution interests here. It's annoying for the weird stepping described in the issue/report to exist. However, if a developer passes an invalid pointer into the relevant function and there's a crash, with this patch they won't get a source location for where that crash came from which is catastrophic for their ability to debug what went wrong. They might not have expected their loop to become a memcpy and we shouldn't force developers to guess what the compiler did to their code.

I think we should collect these defects together to motivate future stepping improvements, but I don't think LLVMs debug-info is expressive enough to fix this problem right now :/. Dropping the location of an instruction that accesses memory is something to seriously avoid if we can. (It happens elsewhere, but typically where the penalty is worse than suboptimal stepping).

We've got some plans to consider stepping/attribution in the future, but we're not there yet.

@Apochens
Copy link
Contributor Author

Apochens commented Feb 23, 2024

Thanks for your careful review! I refined the patch according to the helpful comments. However, as you said, maybe this is not a good idea to accept this patch now. (Hope my refinement to this patch could help.)

@@ -1418,7 +1418,8 @@ bool LoopIdiomRecognize::processLoopStoreOfLoopLoad(
StoreBasePtr, *StoreAlign, LoadBasePtr, *LoadAlign, NumBytes, StoreSize,
AATags.TBAA, AATags.TBAAStruct, AATags.Scope, AATags.NoAlias);
}
NewCall->setDebugLoc(TheStore->getDebugLoc());

NewCall->dropLocation();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be good to document why dropping the location is the right thing to do here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I put a comment here to explain why we need to drop the debug location instead of preserving the debug location of TheStore.

define dso_local void @fun(ptr noalias noundef %a, ptr noalias noundef %b) #0 !dbg !10 {

; CHECK-LABEL: entry:
; CHECK: call void @llvm.memcpy.p0.p0.i64{{\(.*\)$}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, can you comment what is being tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I add the comment to explain why and what it checks.

!21 = distinct !DILexicalBlock(scope: !10, file: !1, line: 2, column: 5)
!22 = !DIBasicType(name: "long", size: 64, encoding: DW_ATE_signed)
!23 = !DILocation(line: 0, scope: !21)
!24 = !DILocation(line: 2, column: 5, scope: !21)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably significantly reduce the size of this testcase by merging most of the DILocations and removing unnecessary variables and !dbg locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! To make the test brief and short, I removed the function main and the redundant debug metadata.

This reverts commit c71d736.
@dwblaikie
Copy link
Collaborator

Do we not have any instructions in the loop preheader we could use as the location for this instruction?

So we looked at a loop, and realized we could map it to a memcpy - I guess not all cases have any instructions in the preheader? (eg: void f(char* a, char* b) { for (;*a; ++a, ++b) { *b = *a; } or something, where there's no initial a = &some_stuff or equivalent) In the case where there is a preheader/initialization, perhaps we should use that location?

Otherwise removing the location is fine/necessary, and we'd still probably get a "flow on" location which might be usable enough.

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.

[DebugInfo] Wrong debug location updates in LoopIdiomRecognize
6 participants