This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Fix backward copy propagation with -g
AbandonedPublic

Authored by condy on Mar 11 2021, 2:04 AM.

Details

Reviewers
None
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.

Diff Detail

Event Timeline

condy created this revision.Mar 11 2021, 2:04 AM
condy requested review of this revision.Mar 11 2021, 2:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 2:04 AM
condy edited the summary of this revision. (Show Details)Mar 11 2021, 2:10 AM
fhahn added a subscriber: fhahn.Mar 11 2021, 2:21 AM

please add a unit test

condy edited the summary of this revision. (Show Details)Mar 11 2021, 2:25 AM
condy added a comment.Mar 11 2021, 2:28 AM

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 added a subscriber: yshui.Mar 11 2021, 3:05 AM
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 added a comment.Mar 11 2021, 3:46 AM
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 added a comment.Mar 11 2021, 3:53 AM

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

condy added a comment.Mar 11 2021, 3:58 AM

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.

please add a unit test

Hi @fhahn did you mean regression/lit 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.

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?

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

-eric

please add a unit test

Hi @fhahn did you mean regression/lit test?

yep. exactly, thanks for the clarification!

fhahn added a comment.Mar 11 2021, 2:10 PM

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 added a comment.EditedMar 12 2021, 1:55 AM

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 added inline comments.Mar 12 2021, 2:54 AM
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 updated this revision to Diff 330206.Mar 12 2021, 4:10 AM

Add a regression test

condy added a comment.Mar 12 2021, 4:13 AM

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

Orlando added inline comments.Mar 12 2021, 4:33 AM
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?

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 added inline comments.Mar 12 2021, 9:29 AM
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 added a comment.EditedMar 12 2021, 10:55 AM

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 marked an inline comment as done.Mar 15 2021, 12:22 AM
condy added inline comments.Mar 15 2021, 1:27 AM
llvm/test/CodeGen/X86/machine-cp-clobbers.mir
10–11 ↗(On Diff #330206)

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

condy updated this revision to Diff 330574.Mar 15 2021, 1:29 AM

CHECK-NEXT directive should be used

condy marked an inline comment as done.Mar 15 2021, 1:29 AM
condy updated this revision to Diff 330575.Mar 15 2021, 1:52 AM

Update test

lkail added a subscriber: lkail.Mar 15 2021, 2:31 AM
lkail added a comment.EditedMar 15 2021, 4:41 AM
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 added inline comments.Mar 15 2021, 1:01 PM
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 added inline comments.Mar 15 2021, 1:46 PM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
655–657

This is what I was getting at, yes.

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

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

Great, I will abandon this patch set.

condy abandoned this revision.Mar 15 2021, 11:16 PM
condy updated this revision to Diff 330899.Mar 16 2021, 1:30 AM

Move test to machine-copy-props.mir

aprantl added inline comments.
llvm/lib/CodeGen/MachineCopyPropagation.cpp
835

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

// Ignore DBG_VALUEs and other meta info.
condy abandoned this revision.Aug 10 2021, 2:13 AM

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