[PATCH] D86813: [DebugInstrRef][2/3] Track PHI values through register coalescing

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:138
+    class PHIValPos {
+    public:
+      SlotIndex SI;    /// Slot where this PHI occurs.
----------------
`struct PHIValPos` ?


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:139
+    public:
+      SlotIndex SI;    /// Slot where this PHI occurs.
+      Register Reg;    /// VReg the PHI occurs in.
----------------
Either 
```
 /// Slot where this PHI occurs.
 SlotIndex SI;
```
or
```
 SlotIndex SI;    ///< Slot where this PHI occurs.
```


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:145
+    /// Map from debug instruction number to PHI position during coalescing.
+    std::map<unsigned, PHIValPos> PHIValToPos;
+    /// Index of, for each VReg, which debug instruction numbers and
----------------
Do we need the sorted-ness property of the std::map or should this be a DenseMap?


================
Comment at: llvm/lib/CodeGen/RegisterCoalescer.cpp:3523
+    else
+      RegToPHIIdx.insert(std::make_pair(CP.getDstReg(), InstrNums));
+  }
----------------
RegToPHIIdx.insert({CP.getDstReg(), InstrNums});


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/phi-coalesce-subreg.mir:66
+  
+  attributes #0 = { noinline nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="all" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+  attributes #1 = { nounwind readnone speculatable willreturn }
----------------
are all those attributes necessary?
Nevermind, you already mentioned that in the description...


================
Comment at: llvm/test/DebugInfo/MIR/InstrRef/phi-coalesce-subreg.mir:101
+  !26 = !DILocation(line: 13, column: 10, scope: !7)
+  !27 = !DILocation(line: 13, column: 3, scope: !7)
+
----------------
maybe remove all unneeded DILocations from the test?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86813/new/

https://reviews.llvm.org/D86813