[CodeGen] Fix backward copy propagation with -g

[condy]

Summary

The bug ticket backward copy propagation failed w/ -g

1856B   renamable $rbp = nuw LEA64r %stack.5.agg.tmp.i, 1, $noreg, 8, $noreg
    DBG_VALUE $rbp, $noreg, !"this", !DIExpression(), debug-location !293; hot.cpp:0 @[ hot.cpp:84:39 @[ hot.cpp:108:11 ] ] line no:0
1864B   renamable $rbx = COPY killed renamable $rbp

Current backward copy propagation does clobber $rbp early when it sees DBG_VALUE which makes it failed to propagate. DebugInfo shouldn't affect opt.

After machine-cp pass

renamable $rbx = nuw LEA64r %stack.5.agg.tmp.i, 1, $noreg, 8, $noreg
DBG_VALUE $rbp, $noreg, !"this", !DIExpression(), debug-location !293; hot.cpp:0 @[ hot.cpp:84:39 @[ hot.cpp:108:11 ] ] line no:0

The DBG_VALUE instruction still remains here.

clang++ hot.cc -o dbg.o -O3 -g
llvm-dwarfdump dbg.o

shows that the %rbp formal parameter debug entry is erased.

[fhahn]

please add a unit test

[condy]

In D98401#2618987, @fhahn wrote:

please add a unit test

Sorry, it's my first patch to LLVM. Let me figure out how to add unit tests. Any tutorials will help me a lot.

Thanks in advance.

[yshui]

renamable $rbx = nuw LEA64r %stack.5.agg.tmp.i, 1, $noreg, 8, $noreg
DBG_VALUE $rbp, $noreg, !"this", !DIExpression(), debug-location !293; hot.cpp:0 @[ hot.cpp:84:39 @[ hot.cpp:108:11 ] ] line no:0

Shouldn't the $rbp in DBG_VALUE be updated as well?

[condy]

In D98401#2619087, @yshui wrote:

renamable $rbx = nuw LEA64r %stack.5.agg.tmp.i, 1, $noreg, 8, $noreg
DBG_VALUE $rbp, $noreg, !"this", !DIExpression(), debug-location !293; hot.cpp:0 @[ hot.cpp:84:39 @[ hot.cpp:108:11 ] ] line no:0

Shouldn't the $rbp in DBG_VALUE be updated as well?

Idealy the DBG_VALUE $rbp should be removed since the use of $rbp is eliminated via machine-cp.
But I notice that even DBG_VALUE $rbp instruction retains, there is no formal parameter $rbp in DWARF output. I guess the debug info module handles such a case.

[yshui]

Because you replaced $rbp with $rbx, surely you would have to replace it everywhere. Removing the DBG_VALUE is potential information loss.

[condy]

In D98401#2619176, @yshui wrote:

Because you replaced $rbp with $rbx, surely you would have to replace it everywhere. Removing the DBG_VALUE is potential information loss.

😢 Right, my brain is down. trying to solve it in the next patch update.

[Orlando]

In D98401#2618987, @fhahn wrote:

please add a unit test

Hi @fhahn did you mean regression/lit test?

In D98401#2619021, @condy wrote:

Sorry, it's my first patch to LLVM. Let me figure out how to add unit tests. Any tutorials will help me a lot.

Thanks in advance.

This should help get you started with writing a test: https://llvm.org/docs/TestingGuide.html

llvm/lib/CodeGen/MachineCopyPropagation.cpp

655–657

Is it worth using MachineInstr::isMetaInstruction instead of MachineInstr::isDebugValue to catch other MIR instructions which don't produce executable code too?

[echristo]

So, why these locations rather than somewhere else? (Also lint checks/testcase/etc).

-eric

[fhahn]

In D98401#2619833, @Orlando wrote:

In D98401#2618987, @fhahn wrote:

please add a unit test

Hi @fhahn did you mean regression/lit test?

yep. exactly, thanks for the clarification!

[fhahn]

You can look at some of the existing tests for machine-cp as inspiration (grep for machine-cp in llvm/test/Codegen) ,e.g. see https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/machine-cp-clobbers.mir

For more background about Machine IR & using it for testing, please take a look at https://llvm.org/docs/MIRLangRef.html#mir-testing-guide

[condy]

In D98401#2620620, @fhahn wrote:

You can look at some of the existing tests for machine-cp as inspiration (grep for machine-cp in llvm/test/Codegen) ,e.g. see https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/AArch64/machine-cp-clobbers.mir

For more background about Machine IR & using it for testing, please take a look at https://llvm.org/docs/MIRLangRef.html#mir-testing-guide

Thanks, I follow the tutorial to create a MIR test finnaly.

# RUN: llc -mtriple=x86_64 -run-pass=machine-cp %s -o - | FileCheck %s

---
name: dont_clobber_register_in_dbg_value
tracksRegLiveness: false
body: |
  bb.0:
    ; CHECK-LABEL: name: dont_clobber_register_in_dbg_value
    ; CHECK: renamable $rbx = LEA64r $rax, 1, $noreg, 8, $noreg
    ; CHECK: DBG_VALUE $rbp, $noreg
    ; CHECK: DBG_VALUE $noreg, $noreg
    renamable $rbx = LEA64r $rax, 1, $noreg, 8, $noreg
    DBG_VALUE $rbp, $noreg
    renamable $rbx = COPY killed renamable $rbp
    DBG_VALUE $noreg, $noreg

...

[condy]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

655–657

It should be. Actually I don't know there is a function isMetaInstruction, thanks for pointing out.

[condy]

Add a regression test

[condy]

The debuginfo should be udpate. It will be implemented in the next patch update.

[Orlando]

llvm/test/CodeGen/X86/machine-cp-clobbers.mir

10–11 ↗

(On Diff #330206)

Just to check I understand, renamable $rbx = COPY killed renamable $rbp should be removed by this pass (with your patch applied) because it's a redundant copy, right?

I think these should be CHECK-NEXT directives (see https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-next-directive). Otherwise the test will pass even if renamable $rbx = COPY killed renamable $rbp is not removed.

15 ↗

(On Diff #330206)

Please can you update the test to show that @yshui's concern (https://reviews.llvm.org/D98401?vs=329886&id=330206#2619087) has been addressed?

For example, if you change this line to DBG_VALUE $rbx, $noreg and update the final CHECK directive to CHECK: DBG_VALUE $rbp, $noreg to show that the DBG_VALUE's register gets updated when the COPY is removed, does the test still pass?

[Orlando]

In D98401#2621811, @condy wrote:

The debuginfo should be udpate. It will be implemented in the next patch update.

Ah, sorry, I posted my comments on the test before seeing your comment above!

[fhahn]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

655–657

Can this check be at the beginning of the loop? (same for the other one).

There also are iterators to skip over debug instructions (instructionsWithoutDebug), perhaps a version that skips meta instructions might also be helpful.

[yshui]

I think it might be valuable to extend this to all cases like this:

$reg1 = ...
USE $reg1
... // No writes to $reg1, and no reads of or writes to $reg2
$reg2 = COPY killed $reg1

Which can be rewritten to

$reg2 = ...
USE $reg2
...

Does this idea make sense? I am currently working on a patch that does this.

[condy]

llvm/test/CodeGen/X86/machine-cp-clobbers.mir

10–11 ↗

(On Diff #330206)

Yes, the last two CHECKs should be CHECK-NEXT.

[condy]

CHECK-NEXT directive should be used

[condy]

Update test

[lkail]

In D98401#2619087, @yshui wrote:

renamable $rbx = nuw LEA64r %stack.5.agg.tmp.i, 1, $noreg, 8, $noreg
DBG_VALUE $rbp, $noreg, !"this", !DIExpression(), debug-location !293; hot.cpp:0 @[ hot.cpp:84:39 @[ hot.cpp:108:11 ] ] line no:0

Shouldn't the $rbp in DBG_VALUE be updated as well?

I agree with @yshui that DBG_VALUE should be updated since we want to keep codegen w/o -g as consistent as possible.

I think backward propagation is missing tracking dbg values as forward propagation does which uses a CopyDbgUsers map to achieve.

[yshui]

llvm/test/CodeGen/X86/machine-cp-clobbers.mir

1 ↗

(On Diff #330575)

This should probably be added to llvm/test/CodeGen/X86/machine-copy-prop.mir

[echristo]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

655–657

This is what I was getting at, yes.

Also, style nit: comments are complete sentences, etc.

[yshui]

https://reviews.llvm.org/D98659 could supersede this

[condy]

In D98401#2627512, @yshui wrote:

https://reviews.llvm.org/D98659 could supersede this

Great, I will abandon this patch set.

[condy]

Move test to machine-copy-props.mir

[aprantl]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

835

Nit: LLVM coding style prefers full sentences, i.e.:

// Ignore DBG_VALUEs and other meta info.

[condy]

Since it has been resolved in https://reviews.llvm.org/D104394, close it.

llvm/lib/CodeGen/MachineCopyPropagation.cpp

 void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
   LLVM_DEBUG(dbgs() << "MCP: ForwardCopyPropagateBlock " << MBB.getName()
                     << "\n");
 
   for (MachineBasicBlock::iterator I = MBB.begin(), E = MBB.end(); I != E; ) {
     MachineInstr *MI = &*I;
     ++I;
 
+    // Ignore DBG_VALUEs and other meta info
+    if (MI->isMetaInstruction())
+      continue;
+
     // Analyze copies (which don't overlap themselves).
     if (MI->isCopy() && !TRI->regsOverlap(MI->getOperand(0).getReg(),
                                           MI->getOperand(1).getReg())) {
       assert(MI->getOperand(0).getReg().isPhysical() &&
              MI->getOperand(1).getReg().isPhysical() &&
              "MachineCopyPropagation should be run after register allocation!");
 
       MCRegister Def = MI->getOperand(0).getReg().asMCReg();
if (MI->isCopy() && !TRI->regsOverlap(MI->getOperand(0).getReg(),
         Tracker.clobberRegister(Reg, *TRI);
       }
 
       Tracker.trackCopy(MI, *TRI);
 
       continue;
     }
 
     // Clobber any earlyclobber regs first.
     for (const MachineOperand &MO : MI->operands())
       if (MO.isReg() && MO.isEarlyClobber()) {
         MCRegister Reg = MO.getReg().asMCReg();
         // If we have a tied earlyclobber, that means it is also read by this
         // instruction, so we need to make sure we don't remove it as dead
         // later.
         if (MO.isTied())
           ReadRegister(Reg, *MI, RegularUse);
         Tracker.clobberRegister(Reg, *TRI);
       }
void MachineCopyPropagation::BackwardCopyPropagateBlock(
   LLVM_DEBUG(dbgs() << "MCP: BackwardCopyPropagateBlock " << MBB.getName()
                     << "\n");
 
   for (MachineBasicBlock::reverse_iterator I = MBB.rbegin(), E = MBB.rend();
        I != E;) {
     MachineInstr *MI = &*I;
     ++I;
 
+    // Ignore DBG_VALUEs and other meta info
+    if (MI->isMetaInstruction())
+      continue;
+
     // Ignore non-trivial COPYs.
     if (MI->isCopy() && MI->getNumOperands() == 2 &&
         !TRI->regsOverlap(MI->getOperand(0).getReg(),
                           MI->getOperand(1).getReg())) {
 
       MCRegister Def = MI->getOperand(0).getReg().asMCReg();
       MCRegister Src = MI->getOperand(1).getReg().asMCReg();
 

llvm/test/CodeGen/X86/machine-copy-prop.mir

 # RUN: llc -mtriple=i686-- -run-pass machine-cp -verify-machineinstrs -o - %s | FileCheck %s
 
 --- |
   declare void @foo()
   define void @copyprop_remove_kill0() { ret void }
   define void @copyprop_remove_kill1() { ret void }
   define void @copyprop_remove_kill2() { ret void }
   define void @copyprop0() { ret void }
   define void @copyprop1() { ret void }
   define void @copyprop2() { ret void }
   define void @nocopyprop0() { ret void }
   define void @nocopyprop1() { ret void }
   define void @nocopyprop2() { ret void }
   define void @nocopyprop3() { ret void }
   define void @nocopyprop4() { ret void }
   define void @nocopyprop5() { ret void }
+  define void @dont_clobber_register_in_dbg_value() { ret void }
 ...
 ---
 # The second copy is redundant and will be removed, check that we also remove
 # the kill flag of intermediate instructions.
 # CHECK-LABEL: name: copyprop_remove_kill0
 # CHECK: bb.0:
 # CHECK-NEXT: $rax = COPY $rdi
 # CHECK-NEXT: NOOP implicit $rdi

 # CHECK-NEXT: $rip = COPY $rax
 # CHECK-NEXT: $rip = COPY $rax
 name: nocopyprop5
 body: |
   bb.0:
     $rip = COPY $rax
     $rip = COPY $rax
 ...
+---
+# Don't clobber register when it's in DBG_VALUE.
+# CHECK-LABEL: name: dont_clobber_register_in_dbg_value
+# CHECK: renamable $rbx = LEA64r $rax, 1, $noreg, 8, $noreg
+# CHECK-NEXT: DBG_VALUE $rbp, $noreg
+# CHECK-NEXT: DBG_VALUE $noreg, $noreg
+name: dont_clobber_register_in_dbg_value
+tracksRegLiveness: false
+body: |
+  bb.0:
+    renamable $rbp = LEA64r $rax, 1, $noreg, 8, $noreg
+    DBG_VALUE $rbp, $noreg
+    renamable $rbx = COPY killed renamable $rbp
+    DBG_VALUE $noreg, $noreg
+...