Peephole optimization causes different code generation at "-O2 -g" and "-O2".

[kromanova]

Summary

Fix for BZ #18590: http://llvm.org/bugs/show_bug.cgi?id=18590

This code review is for the bug fix in peephole optimization that folds a load which defines one vreg into the one and only use of that vreg. With debug info, a DBG_VALUE that referenced the vreg considered to be a use, preventing the optimization.

The fix is to ignore DBG_VALUE's during the optimization, and undef a DBG_VALUE that references a vreg that gets removed.

The main issue is how to update the DBG_VALUE properly. I looked at all calls of isDebugValue() to see what is typically done. There is at least one case that does each of the following:

• rewrite the debug info (this was during inlining to change the vreg)
• delete it (happens in tail duplication)
• undef's it, by setting register to %noreg (in eliminate dead insns)

Looking at those also showed that most optimization happily just skip any DBG_VALUE while iterating over insns. Which is what I do in this fix.

I also looked at all calls to MachineInstr::eraseFromParent() to see how other code dealt with DBG_VALUE's when deleting an instruction. In most cases it did nothing. Presumably because the live debug variable pass will clean it up. I tested a fix that does nothing to the DBG_VALUE for this peephole case and it does get set to %noreg by the live debug variable pass.

In the end this fix requires 3 simple changes:

• skip DBG_VALUE's while iterating over instructions
• only consider NonDBG uses when checking whether a def has a single use
• undef any DBG_VALUE's that use the vreg getting folded.

Fix by Trevor Smigiel.

[kromanova]

Accidentally removed a line from PeepholeOptimizer.cpp header. Restored it back.

[echristo]

lib/CodeGen/PeepholeOptimizer.cpp

598 ↗

(On Diff #7097)

"effect" -> "affect"

641 ↗

(On Diff #7097)

Elaborate here.

659 ↗

(On Diff #7097)

I agree with Manman here on this - is there some more general way of doing this since we'll want to do it across multiple passes I imagine. It needs more comments for sure.

[kromanova]

Attached is the updated patch that addresses the issues raised:

• refactor the code that undefs DBG_VALUE into a function
  • the implementation is in MachineRegisterInfo
  • I could not see any other reasonably obvious place to add it
• remove unnecessary debug info from test cases
• improve comments

[kromanova]

Hi Eric,
Does the new patch look good? Can I check in?
Thanks!
Katya.

[echristo]

This looks fine. If the testcases from the other patch end up changing based on Chandler's review then go ahead and update these as well please?

You might want to verify that the code generation for your testcases doesn't change on an atom processor :)

Thanks!

-eric

[kromanova]

Hi Eric,
Out of curiosity: what is so special about Atom? Are we talking about X86 or x86_64?

Just to confirm that we are on the same page...
You would like me to add one more check to the test. Something like that:
; RUN: llc < %s -mcpu=atom -mtriple=i686-linux | FileCheck %s

Should I do the same check (for Atom) for the other -g -O2/-O2 difference bug fix that I filed?
http://llvm-reviews.chandlerc.com/D2970

Thanks!
Katya.

[kromanova]

Here is an updated patch.

• it has been built and tested against trunk@203192 (Mar6)
• the 2 test cases were merged to 1, and debug info reduced a bit more.
• the test case was checked with -mcpu=atom

[kromanova]

OK to commit?

[echristo]

LGTM. Thanks for all of the work, it's really appreciated.

[Eugene.Zelenko]

Committed in rL203829.

llvm/include/llvm/CodeGen/MachineRegisterInfo.h

     return Hint.first ? 0 : Hint.second;
   }
 
+  /// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
+  /// specified register as undefined which causes the DBG_VALUE to be
+  /// deleted during LiveDebugVariables analysis.
+  void markUsesInDebugValueAsUndef(unsigned Reg) const;
 
   //===--------------------------------------------------------------------===//
   // Physical Register Use Info

llvm/lib/CodeGen/DeadMachineInstructionElim.cpp

           unsigned Reg = MO.getReg();
           if (!TargetRegisterInfo::isVirtualRegister(Reg))
             continue;
-          MachineRegisterInfo::use_iterator nextI;
-          for (MachineRegisterInfo::use_iterator I = MRI->use_begin(Reg),
-               E = MRI->use_end(); I!=E; I=nextI) {
-            nextI = std::next(I);  // I is invalidated by the setReg
-            MachineOperand& Use = I.getOperand();
-            MachineInstr *UseMI = Use.getParent();
-            if (UseMI==MI)
-              continue;
-            assert(Use.isDebug());
-            UseMI->getOperand(0).setReg(0U);
-          }
+          MRI->markUsesInDebugValueAsUndef(Reg);
         }
         AnyChanges = true;
         MI->eraseFromParent();

llvm/lib/CodeGen/MachineRegisterInfo.cpp

       return false;
   return true;
 }
+
+/// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
+/// specified register as undefined which causes the DBG_VALUE to be
+/// deleted during LiveDebugVariables analysis.
+void MachineRegisterInfo::markUsesInDebugValueAsUndef(unsigned Reg) const {
+  // Mark any DBG_VALUE that uses Reg as undef (but don't delete it.)
+  MachineRegisterInfo::use_iterator nextI;
+  for (use_iterator I = use_begin(Reg), E = use_end(); I != E; I = nextI) {
+    nextI = std::next(I);  // I is invalidated by the setReg
+    MachineOperand& Use = I.getOperand();
+    MachineInstr *UseMI = Use.getParent();
+    if (UseMI->isDebugValue())
+      UseMI->getOperand(0).setReg(0U);
+  }
+}

llvm/lib/CodeGen/PeepholeOptimizer.cpp

     return false;
 
   unsigned Reg = MI->getOperand(0).getReg();
-  // To reduce compilation time, we check MRI->hasOneUse when inserting
+  // To reduce compilation time, we check MRI->hasOneNonDBGUse when inserting
   // loads. It should be checked when processing uses of the load, since
   // uses can be removed during peephole.
   if (!MI->getOperand(0).getSubReg() &&
       TargetRegisterInfo::isVirtualRegister(Reg) &&
-      MRI->hasOneUse(Reg)) {
+      MRI->hasOneNonDBGUse(Reg)) {
     FoldAsLoadDefReg = Reg;
     return true;
   }

       ++MII;
       LocalMIs.insert(MI);
 
+      // Skip debug values. They should not affect this peephole optimization.
+      if (MI->isDebugValue())
+          continue;
+
       // If there exists an instruction which belongs to the following
       // categories, we will discard the load candidate.
       if (MI->isLabel() || MI->isPHI() || MI->isImplicitDef() ||
-          MI->isKill() || MI->isInlineAsm() || MI->isDebugValue() ||
+          MI->isKill() || MI->isInlineAsm() ||
           MI->hasUnmodeledSideEffects()) {
         FoldAsLoadDefReg = 0;
         continue;

       if (!isLoadFoldable(MI, FoldAsLoadDefReg) && FoldAsLoadDefReg) {
         // We need to fold load after optimizeCmpInstr, since optimizeCmpInstr
         // can enable folding by converting SUB to CMP.
+        // Save FoldAsLoadDefReg because optimizeLoadInstr() resets it and we
+        // need it for markUsesInDebugValueAsUndef().
+        unsigned FoldedReg = FoldAsLoadDefReg;
         MachineInstr *DefMI = 0;
         MachineInstr *FoldMI = TII->optimizeLoadInstr(MI, MRI,
                                                       FoldAsLoadDefReg, DefMI);

           LocalMIs.insert(FoldMI);
           MI->eraseFromParent();
           DefMI->eraseFromParent();
+          MRI->markUsesInDebugValueAsUndef(FoldedReg);
           ++NumLoadFold;
 
           // MI is replaced with FoldMI.

llvm/test/CodeGen/X86/dbg-changes-codegen.ll

+; RUN: llc < %s -march=x86-64 | FileCheck %s
+
+; The Peephole optimizer should fold the load into the cmp even with debug info.
+; CHECK-LABEL: _ZN3Foo3batEv
+; CHECK-NOT: movq pfoo
+; CHECK: cmpq {{%[a-z]+}}, pfoo(%rip)
+;
+; CHECK-LABEL: _Z3bazv
+; CHECK-NOT: movq wibble2
+; CHECK: cmpq {{%[a-z]+}}, wibble2(%rip)
+
+; Regenerate test with this command: 
+;   clang -emit-llvm -S -O2 -g
+; from this source:
+;   struct Foo {
+;     bool bat();
+;     bool operator==(Foo &arg) { return (this == &arg); }
+;   };
+;   Foo *pfoo;
+;   bool Foo::bat() { return (*this == *pfoo); }
+;
+;   struct Wibble {
+;     int x;
+;   } *wibble1, *wibble2;
+;   struct Flibble {
+;     void bar(Wibble *c) {
+;       if (c < wibble2)
+;         wibble2 = 0;
+;       c->x = 0;
+;     }
+;   } flibble;
+;   void baz() { flibble.bar(wibble1); }
+
+%struct.Foo = type { i8 }
+%struct.Wibble = type { i32 }
+%struct.Flibble = type { i8 }
+
+@pfoo = global %struct.Foo* null, align 8
+@wibble1 = global %struct.Wibble* null, align 8
+@wibble2 = global %struct.Wibble* null, align 8
+@flibble = global %struct.Flibble zeroinitializer, align 1
+
+; Function Attrs: nounwind readonly uwtable
+define zeroext i1 @_ZN3Foo3batEv(%struct.Foo* %this) #0 align 2 {
+entry:
+  %0 = load %struct.Foo** @pfoo, align 8
+  tail call void @llvm.dbg.value(metadata !{%struct.Foo* %0}, i64 0, metadata !62)
+  %cmp.i = icmp eq %struct.Foo* %0, %this
+  ret i1 %cmp.i
+}
+
+; Function Attrs: nounwind uwtable
+define void @_Z3bazv() #1 {
+entry:
+  %0 = load %struct.Wibble** @wibble1, align 8
+  tail call void @llvm.dbg.value(metadata !64, i64 0, metadata !65)
+  %1 = load %struct.Wibble** @wibble2, align 8
+  %cmp.i = icmp ugt %struct.Wibble* %1, %0
+  br i1 %cmp.i, label %if.then.i, label %_ZN7Flibble3barEP6Wibble.exit
+
+if.then.i:                                        ; preds = %entry
+  store %struct.Wibble* null, %struct.Wibble** @wibble2, align 8
+  br label %_ZN7Flibble3barEP6Wibble.exit
+
+_ZN7Flibble3barEP6Wibble.exit:                    ; preds = %entry, %if.then.i
+  %x.i = getelementptr inbounds %struct.Wibble* %0, i64 0, i32 0
+  store i32 0, i32* %x.i, align 4
+  ret void
+}
+
+; Function Attrs: nounwind readnone
+declare void @llvm.dbg.value(metadata, i64, metadata) #2
+
+attributes #0 = { nounwind readonly uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind uwtable "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind readnone }
+
+
+!17 = metadata !{i32 786448, null, null, null, i32 0, i64 0, i64 0, i64 0, i32 0, null} ; [ DW_TAG_reference_type ] [line 0, size 0, align 0, offset 0] [from Foo]
+!45 = metadata !{i32 786447, null, null, metadata !"", i32 0, i64 64, i64 64, i64 0, i32 0, null} ; [ DW_TAG_pointer_type ] [line 0, size 64, align 64, offset 0] [from Flibble]
+!62 = metadata !{i32 786689, null, metadata !"arg", null, i32 33554436, metadata !17, i32 0, null} ; [ DW_TAG_arg_variable ] [arg] [line 4]
+!64 = metadata !{%struct.Flibble* undef}
+!65 = metadata !{i32 786689, null, metadata !"this", null, i32 16777229, metadata !45, i32 1088, null} ; [ DW_TAG_arg_variable ] [this] [line 13]