This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by yuanboli233 on Apr 11 2021, 12:29 PM.

Details

Summary

The bug is proposed here: https://bugs.llvm.org/show_bug.cgi?id=49270
Nobody has commented on the bug yet, but I think it clearly deviates from the description in our LLVM how-to-update-debug-info guide here: https://llvm.org/docs/HowToUpdateDebugInfo.html#when-to-merge-instruction-locations

"(Merges should be applied when) peephole optimizations which combine multiple instructions together, like (add (mul A B) C) => llvm.fma.f32(A, B, C)".

Multiple store instructions in the same basic block are merged into one memset instruction:

In the original ll file:

%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
  tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !17
  ret void, !dbg !18

The ll file after apply -memcpyopt pass:

%arrayidx = getelementptr inbounds i32, i32* %P, i64 1, !dbg !13
  call void @llvm.dbg.value(metadata i32* %arrayidx, metadata !9, metadata !DIExpression()), !dbg !13
  %add.ptr = getelementptr inbounds i32, i32* %P, i64 2, !dbg !14
  call void @llvm.dbg.value(metadata i32* %add.ptr, metadata !11, metadata !DIExpression()), !dbg !14
  %0 = bitcast i32* %add.ptr to i8*, !dbg !15
  call void @llvm.dbg.value(metadata i8* %0, metadata !12, metadata !DIExpression()), !dbg !15
  %1 = bitcast i32* %arrayidx to i8*, !dbg !16
  call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 15, i1 false), !dbg !17
  ret void, !dbg !16

There are two stores: "stores i32 0, i32* %arrayidx, align 4, !dbg !14" and "tail call void @llvm.memset.p0i8.i64(i8* %0, i8 0, i64 11, i1 false), !dbg !17".

They are merged into one memset instruction " call void @llvm.memset.p0i8.i64(i8* align 4 %1, i8 0, i64 15, i1 false), !dbg !17".

The new memset instruction only takes the debug location from one of the merged instructions. However, it should take the merged debug location from the two instructions.

Diff Detail

Event Timeline

yuanboli233 created this revision.Apr 11 2021, 12:29 PM
yuanboli233 requested review of this revision.Apr 11 2021, 12:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2021, 12:29 PM

Sorry for failing the tests, will check the problem and revise to pass the unit tests, it is not happening in my local machine.

djtodoro added inline comments.Apr 11 2021, 11:48 PM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
488

Please use llvm::SmallVector instead.

Please use camel naming convention here.

Thanks for the suggestions from @djtodoro! Format revised and changed to SmallVector.

djtodoro added inline comments.Apr 12 2021, 1:18 AM
llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
492

typo: Dgb

Any how, we don't need the variable, just to it in one line.

Thanks for pointing it out, I have made it to one line.

yuanboli233 marked 2 inline comments as done.Apr 13 2021, 6:47 PM

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

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
491

I think for single-line loops we omit the curly-braces? Run it through clang-format-patch and see what it says.

llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll
3

The test should check that the memset call has this metadata attached to it.

8–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.
25–27

Un-necessary attributes should be deleted from debug-info tests

Updated the test case to include both the preserving case and the merge case.

Code format updated as well.

Thanks for the suggestions from @jmorse!

yuanboli233 marked 4 inline comments as done.Apr 16 2021, 2:38 AM
djtodoro added inline comments.Apr 16 2021, 2:54 AM
llvm/test/Transforms/MemCpyOpt/memset-merged-loc.ll
3

(Super nit)
Instead of ; COM: for comments in tests, we use ;;

42

Since we deleted the attributes, we don't need the #0 anymore.

Thanks for the suggestion from @djtodoro, updated the diff

jmorse accepted this revision.Apr 23 2021, 8:16 AM

LGTM

This revision is now accepted and ready to land.Apr 23 2021, 8:16 AM
djtodoro accepted this revision.Apr 23 2021, 8:18 AM