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

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: lib/CodeGen/MachineInstr.cpp:2029
+void MachineInstr::collectDebugValues(
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  MachineInstr &MI = *this;
----------------
vsk wrote:
> vsk wrote:
> > aprantl wrote:
> > > 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.
> > `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.)
> + @mattd
Thanks for your feedback. It is a good point. I will have a look at it and see if such functionality is available.


================
Comment at: lib/CodeGen/MachineInstr.cpp:2029
+void MachineInstr::collectDebugValues(
+                                SmallVectorImpl<MachineInstr *> &DbgValues) {
+  MachineInstr &MI = *this;
----------------
CarlosAlbertoEnciso wrote:
> vsk wrote:
> > vsk wrote:
> > > aprantl wrote:
> > > > 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.
> > > `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.)
> > + @mattd
> Thanks for your feedback. It is a good point. I will have a look at it and see if such functionality is available.
Thanks for your feedback.

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


================
Comment at: test/Transforms/EarlyCSE/debuginfo-locations-dce.ll:2
+; RUN: llc -O2 %s -o %t -filetype=obj
+; RUN: llvm-dwarfdump -debug-info %t | FileCheck %s
+
----------------
vsk wrote:
> 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.
Thanks for your feedback.

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


Repository:
  rL LLVM

https://reviews.llvm.org/D50887