[PATCH] D100270: [debug-info] MemCpyOpt should merge the debug location when replace multiple block-local instruction with a new memset

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

This is a good find; style nit and test comments inline.



================
Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:491
+        StoreLocs.push_back(StoreInst->getDebugLoc().get());
+      }
+      AMemSet->setDebugLoc(DebugLoc(DILocation::getMergedLocations(StoreLocs)));
----------------
I think for single-line loops we omit the curly-braces? Run it through clang-format-patch and see what it says.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll:2
+; RUN: opt -memcpyopt -S < %s | FileCheck %s
+; CHECK: !{{[0-9]+}} = !DILocation(line: 0, scope: !{{[0-9]+}})
+
----------------
The test should check that the memset call has this metadata attached to it.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll:7-15
+  %arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !13
+  call void @llvm.dbg.value(metadata i32* %arrayidx, metadata !9, metadata !DIExpression()), !dbg !13
+  store i32 0, i32* %arrayidx, align 4, !dbg !14
+  %add.ptr = getelementptr inbounds i32, i32* %P, i64 2, !dbg !15
+  call void @llvm.dbg.value(metadata i32* %add.ptr, metadata !11, metadata !DIExpression()), !dbg !15
+  %0 = bitcast i32* %add.ptr to i8*, !dbg !16
+  call void @llvm.dbg.value(metadata i8* %0, metadata !12, metadata !DIExpression()), !dbg !16
----------------
IMO, there should be more than one store to ensure the new code is being fully covered. Ideally:
 * One set of stores with identical DebugLocs, coalesced into a memset with that DebugLoc,
 * Another set of stores with differing DebugLocs, that should produce a memset where the DebugLoc is for "line: 0", which is what getMergedLocations should produce I think.


================
Comment at: llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll:24-26
+attributes #0 = { ssp }
+attributes #1 = { argmemonly nofree nosync nounwind willreturn writeonly }
+attributes #2 = { nofree nosync nounwind readnone speculatable willreturn }
----------------
Un-necessary attributes should be deleted from debug-info tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100270/new/

https://reviews.llvm.org/D100270