[PATCH] D50887: [DWARF] Missing location debug information with -O2.

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso marked 2 inline comments as done.
CarlosAlbertoEnciso added a comment.

In https://reviews.llvm.org/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.



================
Comment at: lib/CodeGen/MachineInstr.cpp:2037
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  DbgValues.clear();
+  const MachineOperand &MO = getOperand(0);
----------------
vsk wrote:
> While we're improving this utility, wdyt of removing this `clear`? It doesn't seem to be necessary.
You are correct. The clear() is not necessary.


https://reviews.llvm.org/D50887