[DWARF] Missing location debug information with -O2.

[CarlosAlbertoEnciso]

Summary

For the below test:

typedef float __attribute__((__vector_size__(16))) f4;
f4 get();
int main() {
   float MyVar = get()[0];
   if (MyVar)
     return 1;
}

Debug location information for 'MyVar' is missing with -O2.

Check that Machine CSE correctly handles during the transformation, the
debug location information for local variables.

[aprantl]

lib/CodeGen/MachineInstr.cpp

2029 ↗

(On Diff #161183)

I understand that this is not your code, but is there a way to use the list of USEs instead of linearily scanning through all instructions? I'm not sure if MIR has that capability, so it's possible that the answer is no.

[vsk]

Thanks for doing this!

lib/CodeGen/MachineInstr.cpp

2029 ↗

(On Diff #161183)

MachineInstrIterator::use_instructions(Reg) should do it, and a MRI should be available in the contexts where collecting debug values is a useful thing to do. @CarlosAlbertoEnciso would you mind trying that out? It might require moving collectDebugValues elsewhere, but it could be a nice simplification. (I haven't tried this myself, & it's possible this won't work out, in which case we can look at it in a follow-up.)

test/Transforms/EarlyCSE/debuginfo-locations-dce.ll

2 ↗

(On Diff #161183)

Since you're modifying the backend CSE pass, and the test requires a host compiler with X86 support, I think this test belongs in test/CodeGen/X86.

[vsk]

lib/CodeGen/MachineInstr.cpp

2029 ↗

(On Diff #161183)

+ @mattd

[CarlosAlbertoEnciso]

lib/CodeGen/MachineInstr.cpp

2029 ↗

(On Diff #161183)

Thanks for your feedback. It is a good point. I will have a look at it and see if such functionality is available.

2029 ↗

(On Diff #161183)

Thanks for your feedback.

It seems what @aprantl is referring to. I am happy to trying that out.

test/Transforms/EarlyCSE/debuginfo-locations-dce.ll

2 ↗

(On Diff #161183)

Thanks for your feedback.

Good point. I will move the test to that specific location.

[CarlosAlbertoEnciso]

Following feedback from the reviewers:

• Move test case to a more suitable location: test/CodeGen/X86
• Modify 'collectDebugValues' to use MRI USEs.

[vsk]

This is looking really good. I think it's great that the test exercises more than just MachineCSE in isolation. Just two more minor comments (inline) --

include/llvm/CodeGen/MachineInstr.h

1528 ↗

(On Diff #161695)

This function won't work as intended if MRI is null. Could you make it a reference so that future developers know to pass in a valid object?

lib/CodeGen/MachineInstr.cpp

2037 ↗

(On Diff #161695)

While we're improving this utility, wdyt of removing this clear? It doesn't seem to be necessary.

[CarlosAlbertoEnciso]

In D50887#1207849, @vsk wrote:

This is looking really good. I think it's great that the test exercises more than just MachineCSE in isolation. Just two more minor comments (inline) --

Due to an overlooking of my part, the previous patch caused some test cases to fail (check-all). Basically the following code is incorrect:

void MachineInstr::collectDebugValues(MachineRegisterInfo *MRI,
                                SmallVectorImpl<MachineInstr *> &DbgValues) {
  const MachineOperand &MO = getOperand(0);
  if (!MO.isReg())
    return;

  for (MachineInstr &DI : MRI->use_instructions(MO.getReg()))
    if (DI.isDebugValue())
      DbgValues.push_back(&DI);
}

As it is returning all the instructions that use that specific register, causing some tests to fail (accessing invalid instructions).

The original implementation, returns the debug instructions following the given instruction (MI) within the same basic block.

I reviewed the patch, trying to replicate the original list of debug instructions and the additional steps are required:

1. Ignore all instructions before MI
2. Ignore MI
3. Traverse the instructions after MI within the same basic block

I think step (1) adds extra work and makes the function more expensive that the original implementation. Basically, steps (2) and (3) are the original implementation.

There is a similar code that finds the debug instructions in FastISel::sinkLocalValueMaterialization

// Collect all DBG_VALUEs before the new insertion position so that we can
// sink them.
SmallVector<MachineInstr *, 1> DbgValues;
for (MachineInstr &DbgVal : MRI.use_instructions(DefReg)) {
  if (!DbgVal.isDebugValue())
    continue;
  unsigned UseOrder = OrderMap.Orders[&DbgVal];
  if (UseOrder < FirstOrder)
    DbgValues.push_back(&DbgVal);
}

But it uses OrderMap.Orders[&DbgVal] to get the right location in the instructions.

If the goal is to make 'collectDebugValues' more generic, that function should be moved to MachineRegisterInfo and have the steps (1), (2) and (3).

@aprantl, @vsk: before submitting a new patch, I would like to hear your views.

lib/CodeGen/MachineInstr.cpp

2037 ↗

(On Diff #161695)

You are correct. The clear() is not necessary.

[aprantl]

But it uses OrderMap.Orders[&DbgVal] to get the right location in the instructions.

Do you have OrderMap readily available or would you need to compute it first?

If the goal is to make 'collectDebugValues' more generic, that function should be moved to MachineRegisterInfo and have the steps (1), (2) and (3).

That doesn't sound like a bad idea.

[vsk]

@CarlosAlbertoEnciso thanks for investigating this and testing out a new way to collect debug values. Given the complexity of replacing the current algorithm, and that it's not strictly related to fixing MachineCSE, I'd recommend keeping the current algorithm (i.e not using MachineRegisterInfo for now). That will allow us to land a fix in tree soon and hopefully iterate on it.

[CarlosAlbertoEnciso]

In D50887#1212565, @aprantl wrote:

Do you have OrderMap readily available or would you need to compute it first?

At the point where 'collectDebugValues' is called, OrderMap is not available. I need to compute it first.

If the goal is to make 'collectDebugValues' more generic, that function should be moved to MachineRegisterInfo and have the steps (1), (2) and (3).

That doesn't sound like a bad idea.

That would be a good improvement.

[CarlosAlbertoEnciso]

In D50887#1212594, @vsk wrote:

@CarlosAlbertoEnciso thanks for investigating this and testing out a new way to collect debug values. Given the complexity of replacing the current algorithm, and that it's not strictly related to fixing MachineCSE, I'd recommend keeping the current algorithm (i.e not using MachineRegisterInfo for now). That will allow us to land a fix in tree soon and hopefully iterate on it.

I think is the best way to proceed right now, as the patch using the current algorithm is quite simple.

Once the patch is accepted, I would suggest to explore the replacement of the current algorithm to use MachineRegisterInfo and make 'collectDebugValues' a more general function. As @aprantl said, that doesn't sound like a bad idea.

[CarlosAlbertoEnciso]

Update the original patch, as the results from the investigation to replace the algorithm used by 'collectDebugValues', showed some additional complexities.

Replacing that algorithm, seems like a good idea, but not as part of the current issue.

Thanks to the reviewers for their valuable feedback.

[vsk]

Thanks, LGTM. If you don't have commit access yet [1], I'd be happy to commit this on your behalf.

[1] https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

[CarlosAlbertoEnciso]

@vsk & @aprantl Thanks your reviews.

[CarlosAlbertoEnciso]

In D50887#1216171, @vsk wrote:

Thanks, LGTM. If you don't have commit access yet [1], I'd be happy to commit this on your behalf.

[1] https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

Thanks for your offer to commit on my behalf.

I do have commit access but I am having some credential issues, that I am trying to fix. If the problem continues I can ask someone here at the office.

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

if (MO.isReg() && MO.isTied()) {
       getOperand(findTiedOperandIdx(OpIdx)).TiedTo = 0;
       MO.TiedTo = 0;
     }
   }
 
   /// Add all implicit def and use operands to this instruction.
   void addImplicitDefUseOperands(MachineFunction &MF);
 
+  /// Scan instructions following MI and collect any matching DBG_VALUEs.
+  void collectDebugValues(SmallVectorImpl<MachineInstr *> &DbgValues);
+
 private:
   /// If this instruction is embedded into a MachineFunction, return the
   /// MachineRegisterInfo object for the current function, otherwise
   /// return null.
   MachineRegisterInfo *getRegInfo();
 
   /// Unlink all of the register operands in this instruction from their
   /// respective use lists.  This requires that the operands already be on their

llvm/trunk/lib/CodeGen/MachineCSE.cpp

for (MachineOperand &MO : MI->operands()) {
     // To handle it properly we also need a way find a constrained subregister
     // class given a super-reg class and subreg index.
     if (DefMI->getOperand(1).getSubReg())
       continue;
     if (!MRI->constrainRegAttrs(SrcReg, Reg))
       continue;
     LLVM_DEBUG(dbgs() << "Coalescing: " << *DefMI);
     LLVM_DEBUG(dbgs() << "***     to: " << *MI);
+
+    // Collect matching debug values.
+    SmallVector<MachineInstr *, 2> DbgValues;
+    DefMI->collectDebugValues(DbgValues);
+    // Propagate SrcReg to debug value instructions.
+    for (auto *DBI : DbgValues)
+      DBI->getOperand(0).setReg(SrcReg);
+
     // Propagate SrcReg of copies to MI.
     MO.setReg(SrcReg);
     MRI->clearKillFlags(SrcReg);
     // Coalesce single use copies.
     if (OnlyOneUse) {
       DefMI->eraseFromParent();
       ++NumCoalesces;
     }

llvm/trunk/lib/CodeGen/MachineInstr.cpp

 }
 
 void llvm::updateDbgValueForSpill(MachineInstr &Orig, int FrameIndex) {
   const DIExpression *Expr = computeExprForSpill(Orig);
   Orig.getOperand(0).ChangeToFrameIndex(FrameIndex);
   Orig.getOperand(1).ChangeToImmediate(0U);
   Orig.getOperand(3).setMetadata(Expr);
 }
+
+void MachineInstr::collectDebugValues(
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  MachineInstr &MI = *this;
+  if (!MI.getOperand(0).isReg())
+    return;
+
+  MachineBasicBlock::iterator DI = MI; ++DI;
+  for (MachineBasicBlock::iterator DE = MI.getParent()->end();
+       DI != DE; ++DI) {
+    if (!DI->isDebugValue())
+      return;
+    if (DI->getOperand(0).isReg() &&
+        DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
+      DbgValues.push_back(&*DI);
+  }
+}

llvm/trunk/lib/CodeGen/MachineSink.cpp

if (!BreakPHIEdge) {
     }
   }
 
   ToSplit.insert(std::make_pair(FromBB, ToBB));
 
   return true;
 }
 
-/// collectDebgValues - Scan instructions following MI and collect any
-/// matching DBG_VALUEs.
-static void collectDebugValues(MachineInstr &MI,
-                               SmallVectorImpl<MachineInstr *> &DbgValues) {
-  DbgValues.clear();
-  if (!MI.getOperand(0).isReg())
-    return;
-
-  MachineBasicBlock::iterator DI = MI; ++DI;
-  for (MachineBasicBlock::iterator DE = MI.getParent()->end();
-       DI != DE; ++DI) {
-    if (!DI->isDebugValue())
-      return;
-    if (DI->getOperand(0).isReg() &&
-        DI->getOperand(0).getReg() == MI.getOperand(0).getReg())
-      DbgValues.push_back(&*DI);
-  }
-}
-
 /// isProfitableToSinkTo - Return true if it is profitable to sink MI.
 bool MachineSinking::isProfitableToSinkTo(unsigned Reg, MachineInstr &MI,
                                           MachineBasicBlock *MBB,
                                           MachineBasicBlock *SuccToSinkTo,
                                           AllSuccsCache &AllSuccessors) {
   assert (SuccToSinkTo && "Invalid SinkTo Candidate BB");
 
   if (MBB == SuccToSinkTo)
return MBP.LHS.isReg() && MBP.RHS.isImm() && MBP.RHS.getImm() == 0 &&
          MBP.LHS.getReg() == BaseReg;
 }
 
 /// Sink an instruction and its associated debug instructions.
 static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo,
                         MachineBasicBlock::iterator InsertPos) {
   // Collect matching debug values.
   SmallVector<MachineInstr *, 2> DbgValuesToSink;
-  collectDebugValues(MI, DbgValuesToSink);
+  MI.collectDebugValues(DbgValuesToSink);
 
   // If we cannot find a location to use (merge with), then we erase the debug
   // location to prevent debug-info driven tools from potentially reporting
   // wrong location information.
   if (!SuccToSinkTo.empty() && InsertPos != SuccToSinkTo.end())
     MI.setDebugLoc(DILocation::getMergedLocation(MI.getDebugLoc(),
                                                  InsertPos->getDebugLoc()));
   else

llvm/trunk/test/CodeGen/X86/debuginfo-locations-dce.ll

+; RUN: llc -O2 %s -o %t -filetype=obj
+; RUN: llvm-dwarfdump -debug-info %t | FileCheck %s
+
+; Check that Machine CSE correctly handles during the transformation, the
+; debug location information for variables.
+
+; Generated with clang -c -g -O2
+
+; typedef float __attribute__((__vector_size__(16))) f4;
+; f4 get();
+; int main() {
+;   float MyVar = get()[0];
+;   if (MyVar)
+;     return 1;
+; }
+
+; ModuleID = 'test.cpp'
+source_filename = "test.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+define dso_local i32 @main() !dbg !7 {
+entry:
+  %call = tail call <4 x float> @_Z3getv(), !dbg !14
+  %vecext = extractelement <4 x float> %call, i32 0, !dbg !14
+  call void @llvm.dbg.value(metadata float %vecext, metadata !12, metadata !DIExpression()), !dbg !15
+  %tobool = fcmp une float %vecext, 0.000000e+00, !dbg !16
+  %. = zext i1 %tobool to i32, !dbg !18
+  ret i32 %., !dbg !19
+}
+
+declare dso_local <4 x float> @_Z3getv()
+
+declare void @llvm.dbg.value(metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 8.0.0 (trunk 339665)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "test.cpp", directory: ".")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!6 = !{!"clang version 8.0.0 (trunk 339665)"}
+!7 = distinct !DISubprogram(name: "main", scope: !1, file: !1, line: 3, type: !8, isLocal: false, isDefinition: true, scopeLine: 3, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !11)
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!11 = !{!12}
+!12 = !DILocalVariable(name: "MyVar", scope: !7, file: !1, line: 4, type: !13)
+!13 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!14 = !DILocation(line: 4, column: 18, scope: !7)
+!15 = !DILocation(line: 4, column: 9, scope: !7)
+!16 = !DILocation(line: 5, column: 7, scope: !17)
+!17 = distinct !DILexicalBlock(scope: !7, file: !1, line: 5, column: 7)
+!18 = !DILocation(line: 6, column: 5, scope: !17)
+!19 = !DILocation(line: 7, column: 1, scope: !7)
+
+; Look at the debug location information for variable 'MyVar'.
+; Verify that we see a sequence of DI entries, that looks like:
+; DW_TAG_variable
+;   DW_AT_location        (0x00000000
+;     [0x0000000000000009,  0x0000000000000012): DW_OP_reg17 XMM0)
+;   DW_AT_name    ("MyVar")
+
+; CHECK-LABEL: DW_TAG_variable
+; CHECK-NEXT: DW_AT_location{{.*}}
+; CHECK-NEXT: {{.*}}DW_OP_reg17 XMM0
+; CHECK-NEXT: DW_AT_name{{.*}}("MyVar")
+