[Transform][MemCpyOpt] Add missing DebugLoc to %tmpbitcast

[Pierre-vh]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=37967

I need some help to be sure that this fix is correct, even though it's trivial, I am not sure I fixed this in the best possible way.

• Is this a good fix for this issue, or is it just working around it?
• Is the test I wrote good enough? I just tested for the output of -debugify-each, but perhaps it'd be better to check the LLVM IR directly?

[vsk]

Thanks, this is in good shape. A few minor comments inline.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1305

nit, we generally ask contributors to clang-format (just) their diffs, e.g. with

https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/clang-format-diff.py

I personally hook this up to git via .gitconfig:

[alias]
  fmt = ! git diff -U0 --no-color HEAD~1 | ~/src/llvm-project-master/clang/tools/clang-format/clang-format-diff.py -i -p1

& run 'git fmt' before committing.

llvm/test/Transforms/MemCpyOpt/pr37967.ll

4

This check line makes for a nice regression test.

5

Could you change this to a positive test? e.g.

; CHECK-LABEL: define {{.*}} @_Z3bar3FooS_RiS_
; CHECK: [[firstAlloca:%.*]] = alloca %struct.Foo
; CHECK: bitcast i8* [[firstAlloca]] to %struct.Foo*, !dbg

As-written, it might stop being a useful test if the structure of the IR emitted by the pass changes slightly (seems likely, as the bitcast is a no-op?), or if (for some reason) the -check-debugify output changes.

[vsk]

llvm/test/Transforms/MemCpyOpt/pr37967.ll

5

Ah, oops. %1 has a different type after 'opt' runs, so the check line for 'firstAlloca' needs to be fixed up.

[tellenbach]

Hi, thanks for this. The test case look very verbose and can probably be reduced. At least all unnecessary meta-information should be removed, e.g. does it really need all these function attributes?

[Pierre-vh]

Hello,

I updated the code, it should be correctly formatted now.

About the test, I don't know how to simplify it further as I am still pretty new to LLVM IR, if you have suggestions/explanations I'd love to hear them.
Also, I can't write the test you suggested as the output does not have a !dbg attribute (the exact line looks like this, but debugify accept it: internally, it seems to have a DebugInfo node, it's just empty)

%tmpcast = bitcast i8* %1 to %struct.Foo*

[Pierre-vh]

llvm/test/Transforms/MemCpyOpt/pr37967.ll

5

I can't write the test like that as the resulting line looks like this:

%tmpcast = bitcast i8* %1 to %struct.Foo*

It doesn't have a !dbg attribute (but, since debugify doesn't complain, it seems to have a non-null DebugLoc node, my guess is just that it's empty)

[vsk]

@Pierre-vh you can run opt with -debug-pass=Structure to see what it's doing. In this case, it's stripping out all the debug metadata right before printing module IR:

ModulePass Manager
  FunctionPass Manager
    [snip ...]
    Attach debug info to a function
    Phi Values Analysis
    Memory Dependence Analysis
    MemCpy Optimization
    Check debug info from -debugify-function
    Attach debug info to a function
    Module Verifier
    Check debug info from -debugify-function
  Print Module IR

So that's why the 'CHECK' line doesn't match !dbg, it got stripped out. If you change your RUN line to do opt -instcombine -debugify -memcpyopt -check-debugify, opt will produce IR you can write a positive test against.

As for reducing the test, you can try to find a simpler way to trigger this code path in MemCpyOpt. llvm/test/Transforms/MemCpyOpt/memcpy.ll has lots of tidy examples, perhaps running one of those examples through -debugify-each would hit the same bug.

[Pierre-vh]

Hello,

• I made the test as small as I could
• I added 2 run lines, one checks for the correctness of the LLVM IR, the other checks that debugify passes

[vsk]

I’m not sure I understand why two run lines are needed, and neither are what I’d expect. The test has nothing to do with instcombine afaict, and running debugify /after/ memcpyopt is too late (that won’t test anything, as the real pass has already finished before we try attaching debug metadata).

Istm you just need one run line: opt -debugify -memcpyopt -check-debugify

[Pierre-vh]

In D75173#1897944, @vsk wrote:

I’m not sure I understand why two run lines are needed, and neither are what I’d expect. The test has nothing to do with instcombine afaict, and running debugify /after/ memcpyopt is too late (that won’t test anything, as the real pass has already finished before we try attaching debug metadata).

Istm you just need one run line: opt -debugify -memcpyopt -check-debugify

You are right, I just misunderstood how opt worked (I didn't know the passes were run in order of appearance).
Would this better?

; RUN: opt -instcombine -debugify -memcpyopt -check-debugify -S < %s 2>&1 | FileCheck %s

; CHECK: CheckModuleDebugify: PASS

; CHECK-LABEL: define {{.*}} @_Z3bar3Foo
; CHECK: [[target:%.*]] = load i8*, i8** bitcast (%struct.Foo** @a to i8**), align 8, !dbg
; CHECK: %tmpcast = bitcast i8* [[target]] to %struct.Foo*, !dbg

Note that I need -instcombine or else the test does not pass.

[vsk]

In D75173#1897971, @Pierre-vh wrote:

In D75173#1897944, @vsk wrote:

I’m not sure I understand why two run lines are needed, and neither are what I’d expect. The test has nothing to do with instcombine afaict, and running debugify /after/ memcpyopt is too late (that won’t test anything, as the real pass has already finished before we try attaching debug metadata).

Istm you just need one run line: opt -debugify -memcpyopt -check-debugify

You are right, I just misunderstood how opt worked (I didn't know the passes were run in order of appearance).
Would this better?

; RUN: opt -instcombine -debugify -memcpyopt -check-debugify -S < %s 2>&1 | FileCheck %s

; CHECK: CheckModuleDebugify: PASS

; CHECK-LABEL: define {{.*}} @_Z3bar3Foo
; CHECK: [[target:%.*]] = load i8*, i8** bitcast (%struct.Foo** @a to i8**), align 8, !dbg
; CHECK: %tmpcast = bitcast i8* [[target]] to %struct.Foo*, !dbg

That looks perfect, except

Note that I need -instcombine or else the test does not pass.

Hm, it looks like instcombine needs to massage the IR a bit before memcpyopt can "kick in". There's room in llvm for integration tests, but generally we try to make tests for transforms as narrow as possible (typically you don't want a change in instcombine breaking your test that's really for memcpyopt -- but sometimes you do). You can do that here with

opt -S -instcombine pr37967.ll > simplified.ll

Then your test should fully work with 'opt -debugify -memcpyopt -check-debugify'.

[Pierre-vh]

Thank you very much for your comments. This should be good I think.

[vsk]

Lgtm, thanks! I appreciate your patience on this one.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

bool MemCpyOptPass::processByValArgument(CallSite CS, unsigned ArgNo) {
   // not just the defining memcpy.
   MemDepResult SourceDep = MD->getPointerDependencyFrom(
       MemoryLocation::getForSource(MDep), false,
       CS.getInstruction()->getIterator(), MDep->getParent());
   if (!SourceDep.isClobber() || SourceDep.getInst() != MDep)
     return false;
 
   Value *TmpCast = MDep->getSource();
-  if (MDep->getSource()->getType() != ByValArg->getType())
-    TmpCast = new BitCastInst(MDep->getSource(), ByValArg->getType(),
+  if (MDep->getSource()->getType() != ByValArg->getType()) {
+    BitCastInst *TmpBitCast = new BitCastInst(MDep->getSource(), ByValArg->getType(),
                                               "tmpcast", CS.getInstruction());
+    // Set the tmpcast's DebugLoc to MDep's
+    TmpBitCast->setDebugLoc(MDep->getDebugLoc());
+    TmpCast = TmpBitCast;
+  }
 
   LLVM_DEBUG(dbgs() << "MemCpyOptPass: Forwarding memcpy to byval:\n"
                     << "  " << *MDep << "\n"
                     << "  " << *CS.getInstruction() << "\n");
 
   // Otherwise we're good!  Update the byval argument.
   CS.setArgument(ArgNo, TmpCast);
   ++NumMemCpyInstr;

llvm/test/Transforms/MemCpyOpt/pr37967.ll

+; RUN: opt -debugify -memcpyopt -check-debugify -S < %s 2>&1 | FileCheck %s
+
+; CHECK: CheckModuleDebugify: PASS
+
+; CHECK-LABEL: define {{.*}} @_Z3bar3Foo
+; CHECK: [[target:%.*]] = load i8*, i8** bitcast (%struct.Foo** @a to i8**), align 8, !dbg
+; CHECK: %tmpcast = bitcast i8* [[target]] to %struct.Foo*, !dbg
+
+%struct.Foo = type { i64, i64, i64 }
+
+@a = dso_local global %struct.Foo* null, align 8
+
+define dso_local void @_Z3bar3Foo(%struct.Foo* byval(%struct.Foo) align 8 %0) {
+entry:
+  %agg.tmp = alloca %struct.Foo, align 8
+  %1 = load i8*, i8** bitcast (%struct.Foo** @a to i8**), align 8
+  %2 = bitcast %struct.Foo* %agg.tmp to i8*
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 8 dereferenceable(24) %2, i8* nonnull align 8 dereferenceable(24) %1, i64 24, i1 false)
+  call void @_Z3bar3Foo(%struct.Foo* nonnull byval(%struct.Foo) align 8 %agg.tmp)
+  ret void
+}
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* noalias nocapture writeonly, i8* noalias nocapture readonly, i64, i1 immarg) #0