[CodeGen] Avoid handling DBG_VALUE in the LivePhysRegs (addUses,removeDefs,stepForward)

[mattd]

Summary

This patch prevents DBG_VALUE instructions from influencing
LivePhysRegs::stepBackwards and stepForwards. In at least one case,
specifically branch folding, the stepBackwards logic was having an
influence on code generation. The result was that certain code
compiled with '-g -O2' would differ from that compiled with '-O2'
alone. It seems that the original logic, accounting for DBG_VALUE,
was influencing the placement of an IMPLICIT_DEF which had a later
impact on how blocks were processed in branch folding.

[MatzeB]

This is surprising: DBG_VALUE should not have any defs so removeDefs() should have no effect and all uses should be marked with the debug flag so none of them should pass the MachineOperand::readsReg() test in addUses. Could you check whether the debug flag is set correctly? Did you try running your test with -verify-machineinstrs?

[mattd]

In D43850#1021573, @MatzeB wrote:

This is surprising: DBG_VALUE should not have any defs so removeDefs() should have no effect and all uses should be marked with the debug flag so none of them should pass the MachineOperand::readsReg() test in addUses. Could you check whether the debug flag is set correctly? Did you try running your test with -verify-machineinstrs?

Thanks for the reply and helpful response @MatzeB . I do not get any diagnostic issues when running my test with -verify-machineinstrs.

I think what might be happening is that, during tail merging, some undef operands were switched to being non-undef. That wording I ripped from the comment in lib/CodeGen/BranchFolding.cpp: replaceTailWithBranchTo. This the spot in the code where I discovered the issue.

What I noticed is that when I compile one of my tests with '-g -O2', replaceTailWIthBranchTo collects LiveRegs from the 'old' basic block that is losing its tail. That block will eventually jmp to the NewDest basic block. Some of the registers collected in LiveRegs also happen to be live in the NewDest.liveins. Because of this correlation (reg is live-in old and live-in new) an IMPLICIT_DEF is generated in the old basic block. It just happens to be that a DBG_VALUE is associated to that register. Sorry, that might have been hard to follow. Here is some generated MI that I noticed differed between the '-O2' and '-g -O2'. This dump being from '-g -O2'

bb.11:
; predecessors: %bb.10, %bb.12, %bb.14, %bb.16, %bb.18
  successors: %bb.21(0x80000000); %bb.21(200.00%)
  liveins: $rdi
  $al = IMPLICIT_DEF

bb.21 (%ir-block.4):
; predecessors: %bb.20, %bb.11
  successors: %bb.22(0x80000000); %bb.22(200.00%)
  liveins: $rdi, $al
  DBG_VALUE debug-use $al, debug-use $noreg, !"d", !DIExpression(), debug-location !36; line no:6
  renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags

In one of my tests, I noticed that a particular basic block (bb.11) was being removed in -O2 but it was not being removed in '-O2 -g', which had an impact on the way the basic blocks were laid out, and resulted in a code difference. It appeared that this started from an IMPLICIT_DEF being created via replaceTailWithBranchTo, which is what is listed in bb.11 above.

[mattd]

Just pinging this thread. Does this solution look reasonable, or does it look like we might be masking some other problem?

[kparzysz]

The fix looks reasonable to me, but could you create a .mir testcase instead of .ll?

Also, stepForward may require a similar change.

[bjope]

@mattd: Please upload full diffs when requesting a review in the future (see http://llvm.org/docs/Phabricator.html#phabricator-request-review-web).
It is easier to do a proper review when there is some more context available in Phabricator.

[mattd]

Thanks for the feedback, and my apologies for not providing additional context in the diff. The updated patch:

• I added an 'isDebugValue' check to LivePhysRegs::stepForward, I was hesitant to add this, as it never seemed problematic, but it doesn't seem wrong to add the check here either.
• I made the test a .mir test instead of .ll. I tried to reduce this as much as possible.

[kparzysz]

This looks fine to me, but I'd wait a couple of days to see if Matthias has any concerns.

[mattd]

In D43850#1030494, @kparzysz wrote:

This looks fine to me, but I'd wait a couple of days to see if Matthias has any concerns.

Thanks for the review @kparzysz.

[bjope]

lib/CodeGen/LivePhysRegs.cpp

60 ↗

(On Diff #137313)

We should probably skip debug value MIs here as well.
Or is it perhaps more correct to avoid operands with the isDebug() property set?

The current solution of skipping MI.isDebugValue() is skipping instructions.
So we assume that there are no MachineOperands with isDebug() in other instructions. I guess that is the case today, but is it more future proof to not specialize on MachineInstruction kind in these methods, and instead make sure that addUses/removeDefs/stepForward etc are skipping the operands with isDebug property set?

[bjope]

Summary and title should probably be updated (as this impacts more than stepBackwards now).

[kparzysz]

@bjope's comment about checking isDebug is a valid concern, but I think it goes outside of the scope of this patch. I don't remember seeing any checks for isDebug in other code, and going that path would probably need an agreement from more people that this is indeed the right thing to do.

[mattd]

Thanks for the feedback @bjope, @kparzysz. I think @bjope has a good point regarding the MI Operands.
I think we can solve the same issue I'm trying to solve here, by just adding a isDebug check to the operands in addUses.
I'd also add the same check to removeDefs. That would make this patch here moot. If you think that is the more correct
thing to do, and it sounds like it might be, then I'd rather do that.

[kparzysz]

I thought it would be a more widespread problem, but isDebug is actually checked in the nodbg iterator in MRI.

If you want, go ahead with the change to check isDebug instead.

[bjope]

If you do not think it is too much trouble, then I'd welcome the alternative approach.
But skipping early on the MI.isDebugValue() in stepForward/stepBackward is OK as well.

In case LivePhysRegs::addUses is used from other methods (now or in the future) it would be good to make sure that it ignores isDebug() operands. Or at least add a comment in the method description that any user should make sure to check MI.isDebugValue() before the call.

[mattd]

In D43850#1034803, @bjope wrote:

If you do not think it is too much trouble, then I'd welcome the alternative approach.

But skipping early on the MI.isDebugValue() in stepForward/stepBackward is OK as well.

In case LivePhysRegs::addUses is used from other methods (now or in the future) it would be good to make sure that it ignores isDebug() operands. Or at least add a comment in the method description that any user should make sure to check MI.isDebugValue() before the call.

The only calls to addUses are in LivePhysRegs.cpp, but I'm not opposed to future-proofing. Let me test out the proposed changes, and if that works out, I'll update this patch.

[mattd]

Added debug operand checks (isDebug) to: LivePhysRegs: addUses, removeDefs, and stepForward.

This is a more specific patch that addresses the description in the original summary: Avoiding the situation where DBG_VALUE instructions influence codegen.

[mattd]

I'm following up with this patch. Do the most recent changes that I made (adding the isDebug check on the operands, instead of the instruction), look good? Thanks.

[bjope]

Yes, thanks Matt! LGTM.

[mattd]

Great! Thanks!

llvm/trunk/lib/CodeGen/LivePhysRegs.cpp

if (MO.clobbersPhysReg(*LRI)) {
       ++LRI;
   }
 }
 
 /// Remove defined registers and regmask kills from the set.
 void LivePhysRegs::removeDefs(const MachineInstr &MI) {
   for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {
     if (O->isReg()) {
-      if (!O->isDef())
+      if (!O->isDef() || O->isDebug())
         continue;
       unsigned Reg = O->getReg();
       if (!TargetRegisterInfo::isPhysicalRegister(Reg))
         continue;
       removeReg(Reg);
     } else if (O->isRegMask())
       removeRegsInMask(*O);
   }
 }
 
 /// Add uses to the set.
 void LivePhysRegs::addUses(const MachineInstr &MI) {
   for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {
-    if (!O->isReg() || !O->readsReg())
+    if (!O->isReg() || !O->readsReg() || O->isDebug())
       continue;
     unsigned Reg = O->getReg();
     if (!TargetRegisterInfo::isPhysicalRegister(Reg))
       continue;
     addReg(Reg);
   }
 }
 

 /// Simulates liveness when stepping forward over an instruction(bundle): Remove
 /// killed-uses, add defs. This is the not recommended way, because it depends
 /// on accurate kill flags. If possible use stepBackward() instead of this
 /// function.
 void LivePhysRegs::stepForward(const MachineInstr &MI,
         SmallVectorImpl<std::pair<unsigned, const MachineOperand*>> &Clobbers) {
   // Remove killed registers from the set.
   for (ConstMIBundleOperands O(MI); O.isValid(); ++O) {
-    if (O->isReg()) {
+    if (O->isReg() && !O->isDebug()) {
       unsigned Reg = O->getReg();
       if (!TargetRegisterInfo::isPhysicalRegister(Reg))
         continue;
       if (O->isDef()) {
         // Note, dead defs are still recorded.  The caller should decide how to
         // handle them.
         Clobbers.push_back(std::make_pair(Reg, &*O));
       } else {

llvm/trunk/test/CodeGen/X86/dbg-changes-codegen-branch-folding2.mir

+# RUN: llc -run-pass=block-placement %s -o - | FileCheck %s
+
+--- |
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  define void @_Z3fn1v() !dbg !6 {
+  entry:
+    %d = alloca i8, align 1
+    br i1 1, label %for.cond.cleanup, label %for.body
+  
+  for.cond.cleanup:
+    ret void
+  
+  for.body:
+    br i1 1, label %for.cond.cleanup, label %for.body.1
+  
+  for.body.1:
+    br i1 1, label %for.cond.cleanup, label %for.body.2
+  
+  for.body.2:
+    br i1 1, label %for.cond.cleanup, label %for.body.3
+  
+  for.body.3:
+    br i1 1, label %for.cond.cleanup, label %for.body.4
+  
+  for.body.4:
+    br i1 1, label %for.cond.cleanup, label %for.body.5
+  
+  for.body.5:
+    br i1 1, label %for.cond.cleanup, label %for.body.6
+  
+  for.body.6:
+    br i1 1, label %for.cond.cleanup, label %for.body.7
+  
+  for.body.7:
+    br i1 1, label %for.cond.cleanup, label %for.body.8
+  
+  for.body.8:
+    br i1 1, label %for.cond.cleanup, label %for.body.9
+  
+  for.body.9:
+    br i1 1, label %for.cond.cleanup, label %for.body.10
+  
+  for.body.10:
+    br i1 1, label %for.cond.cleanup, label %for.body.11
+  
+  for.body.11:
+    %d.0.d.0..12 = load volatile i8, i8* %d, align 1
+    call void @llvm.dbg.value(metadata i8 %d.0.d.0..12, metadata !16, metadata !DIExpression()), !dbg !19
+    br label %for.cond.cleanup
+  }
+  
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 7.0.0 (trunk 326606)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, globals: !2)
+  !1 = !DIFile(filename: "repro.cpp", directory: "/home/mdavis/bugs/bz-189869")
+  !2 = !{}
+  !3 = !{i32 2, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = distinct !DISubprogram(name: "fn1", linkageName: "_Z3fn1v", scope: !7, file: !7, line: 4, type: !8, isLocal: false, isDefinition: true, scopeLine: 4, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !15)
+  !7 = !DIFile(filename: "./repro.cpp", directory: "/home/mdavis/bugs/bz-189869")
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{}
+  !15 = !{!16}
+  !16 = !DILocalVariable(name: "d", scope: !6, file: !7, line: 6, type: !17)
+  !17 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !18)
+  !18 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+  !19 = !DILocation(line: 6, column: 17, scope: !6)
+
+...
+---
+# CHECK: name: _Z3fn1v
+# CHECK: bb.10.for.body.9
+# CHECK: renamable $al
+# CHECK-NEXT: TEST8rr killed renamable $al
+# CHECK-NEXT: JNE_1
+# CHECK-NOT: $al = IMPLICIT_DEF
+# CHECK: bb.12.for.body.10
+
+name:            _Z3fn1v
+alignment:       4
+tracksRegLiveness: true
+constants:       
+body:             |
+  bb.0.entry:
+    successors: %bb.1, %bb.4
+    liveins: $rdi, $rbp, $r15, $r14, $r13, $r12, $rbx
+  
+    renamable $al = MOV8ri 1
+    TEST8rr renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.4, implicit killed $eflags
+  
+  bb.1.for.cond.cleanup:
+    successors: %bb.3, %bb.2
+    liveins: $ecx, $rdi
+  
+    renamable $eax = MOV32rm $rsp, 1, $noreg, -16, $noreg 
+    CMP32rm killed renamable $eax, $rip, 1, $noreg, $noreg, $noreg, implicit-def $eflags 
+    JBE_1 %bb.3, implicit $eflags
+  
+  bb.2:
+    successors: %bb.3
+    liveins: $ebp, $ebx, $edx, $esi, $rdi, $r8d, $r9d, $r10d, $r11d, $r12d, $r13d, $r14d, $r15d
+  
+  bb.3.for.cond.cleanup:
+    liveins: $rdi, $xmm3, $xmm4, $xmm5, $xmm6, $xmm7, $xmm9, $xmm13, $xmm14
+    RETQ
+  
+  bb.4.for.body:
+    successors: %bb.1, %bb.5
+    liveins: $al, $rdi
+  
+    renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.5.for.body.1:
+    successors: %bb.1, %bb.6
+    liveins: $ecx, $rdi
+  
+    renamable $al = MOV8ri 1
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.6.for.body.2:
+    successors: %bb.1, %bb.7
+    liveins: $ecx, $eflags, $rdi
+  
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.7.for.body.3:
+    successors: %bb.1, %bb.8
+    liveins: $ecx, $rdi
+  
+    renamable $al = MOV8ri 1
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.8.for.body.4:
+    successors: %bb.1, %bb.9
+    liveins: $ecx, $eflags, $rdi
+  
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.9.for.body.5:
+    successors: %bb.1, %bb.10
+    liveins: $ecx, $rdi
+  
+    renamable $al = MOV8ri 1
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.10.for.body.6:
+    successors: %bb.1, %bb.11
+    liveins: $ecx, $eflags, $rdi
+  
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.11.for.body.7:
+    successors: %bb.1, %bb.12
+    liveins: $ecx, $rdi
+  
+    renamable $al = MOV8ri 1
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.12.for.body.8:
+    successors: %bb.1, %bb.13
+    liveins: $ecx, $eflags, $rdi
+  
+    JNE_1 %bb.1, implicit $eflags
+  
+  bb.13.for.body.9:
+    successors: %bb.14, %bb.15
+    liveins: $rdi
+  
+    renamable $al = MOV8ri 1
+    TEST8rr killed renamable $al, renamable $al, implicit-def $eflags
+    JE_1 %bb.15, implicit $eflags
+  
+  bb.14:
+    successors: %bb.1
+    liveins: $rdi
+  
+    renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags
+    JMP_1 %bb.1
+  
+  bb.15.for.body.10:
+    successors: %bb.16, %bb.17
+    liveins: $eflags, $rdi
+  
+    JE_1 %bb.17, implicit killed $eflags
+  
+  bb.16:
+    successors: %bb.1
+    liveins: $rdi
+  
+    JMP_1 %bb.1
+  
+  bb.17.for.body.11:
+    successors: %bb.1
+    liveins: $rdi
+  
+    dead renamable $al = MOV8rm $rsp, 1, $noreg, -121, $noreg 
+    DBG_VALUE debug-use $al, debug-use $noreg, !16, !DIExpression(), debug-location !19
+    renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags
+    JMP_1 %bb.1
+...