[PATCH] D146980: [Assignment Tracking] Coalesce dbg loc definitions with contiguous fragments

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

Good saving; in the future we might find different ways of representing variable fragments and communicating them to LiveDebugValues, or make LDV smarter, but it's good to get this benefit now. I think the more structured fragment model @scott.linder proposed would deliver the sort of guarantees we'd need.

As discussed offline, I think the only thing missing is making this behaviour conditional on instr-ref variable locations being used, to avoid LiveDebugVariables tripping over it. Rather than seeing it as "assignment tracking needs instruction referencing" and jamming it in the frontnend, we can consider it as being "instruction referencing needs this hack to deal with assignment tracking efficiently". That allows other backends to benefit from the work.



================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:585-589
+    // We've inserted the location into the map. The map will have coalesced
+    // adjacent intervals (variable fragments) that describe the same memory
+    // location. Use this knowledge to insert a debug location that describes
+    // that coalesced fragment. This may eclipse other locs we've just
+    // inserted. This is okay as redundant locs will be cleaned up later.
----------------
I wonder if there's a pathological case where we effectively store a history of defs that we then have to manually clear up, at reasonable expense.

Not really a criticism, everything in compilers has a pathological case, and this is clearly a net positive for compilation overall.


================
Comment at: llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp:684-688
       // Shorten `i` so that there's space to insert `f`.
       //      [ f ]
       // [  -   i   -  ]
       // +
       // [ i ][ f ][ i ]
----------------
In this scenario, would I be right in thinking that the code below shortens "i", inserts "f" in the middle, and then IntervalMap will coalesce them back together if they're the same? (Seems like the right behaviour).


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

https://reviews.llvm.org/D146980