Following support from the previous patches in this stack being added for variadic DBG_INSTR_REFs to exist, this patch modifies LiveDebugValues to handle those instructions. Support already exists for DBG_VALUE_LISTs, which covers most of the work needed to handle these instructions; this patch only modifies the transferDebugInstrRef function to correctly track them.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks good with some tiny comments -- as ever it'll want tests, which should be folded into this patch, could you add those please.
This is also reassuringly small, which is encouraging and suggest it composes well with the other variadic work.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
1647–1656 | I believe DenseMap does an initial allocation, and the common case for DBG_INSTR_REF is going to be single value -- could I recommend using a SmallDenseMap instead? It'll expand to be larger when needed. | |
1678–1690 | Aliasing the type name (DbgOp DbgOp) is going to confuse someone at some point, just "Op" perhaps? | |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
1198 | This will want a docu-comment |
Add some lit test coverage, fix a bug with memoization of DBG_PHIs and remove an out-of-date comment.
Looks good, but with what feels like low test coverage: remind me, the plumbing required to track variadic locations in LiveDebugValues already landed, and we have tests to cover those new code paths, yes?
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
639–640 | Definitely wants a comment as to the intention behind this, I'm struggling to work it out. | |
1650 | Recommend const DbgOp & to avoid unnecessary copies cropping up | |
llvm/test/DebugInfo/MIR/InstrRef/variadic-instr-ref.mir | ||
1–3 ↗ | (On Diff #470530) | Super-nit -- I don't like the file name, ideally there would be some kind of indicator that it's touching LiveDebugValues in some way. |
50 ↗ | (On Diff #470530) | Sounds like a DbgEntityHistoryCalculator matter rather than one for LiveDebugValues, right? |
154 ↗ | (On Diff #470530) | Ideally we'd remove these redundant keys (all the false / zero ones) below |
Rebased onto latest master; addressed review comments. The section of code that turned variadic expressions into non-variadic expressions so that they could be recovered as entry values has been extracted into a separate function in DebugInfoMetadata, and another instance of that logic in AsmPrinter has been updated to use this function. If you think that change should be rolled into a separate patch then I'm happy to do so!
Yes - after all the input debug value instructions are converted to the internal representation (DbgValue) in the instruction referencing live debug values, variadic DBG_INSTR_REFs follow the same code paths as DBG_VALUE_LISTs, which had test coverage added when they were introduced. Notably those patches added unit tests, and this patch has nothing to add on top of them. If you think test coverage could be expanded though then I should be able to generate some more cases, it might not be a bad idea to cast a wide net in case there are any rare or insidious errors.
llvm/test/DebugInfo/MIR/InstrRef/variadic-instr-ref.mir | ||
---|---|---|
50 ↗ | (On Diff #470530) | Indeed - there is some clobbering that's handled in LiveDebugValues, and I thought this should be one such case, but after checking over it again it's firmly out of LiveDebugValues' wheelhouse. |
Rebased onto latest main, slightly updated and added unit tests for convertToNonVariadicExpression.
This will want a docu-comment