InstrRefBasedLDV allocates some big tables of ValueIDNum, to store live-in and live-out block values in, that then get passed around as pointers everywhere. This patch wraps the allocation in a std::unique_ptr, names some types based on unique_ptr, and passes references to those around instead. There's no functional change, but it makes it clearer to the reader that references to these tables are borrowed rather than owned, and we get some extra validity assertions too.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nice. I have a few questions / nits but I'm a fan of the change.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
3256–3257 | I don't think you need this explicit clean-up on this code path any longer, since the unique_ptr dtor will handle it for you? | |
3257–3258 | If I'm reading this correctly, emitLocations is becomes responsible for managing the object lifetimes (it deletes things from MOutLocs and MInLocs); should you std::move MOutLocs and MInLocs to signify this? | |
3265–3266 | You don't need to call reset since MOutLocs is scoped to this function (the unqiue_ptr's dtor will clean up for you). |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
2980–2982 | if functions like these took their argument by reference instead of pointer this code might be simpler (*MInLocs[bbnum] instead of needing the .get() call) | |
3159–3166 | prefer std::make_unique if possible | |
3159–3166 | Hmm, this causes a lot of allocations - any chance of this being a single allocation of MaxNumBlocks x NumLocs? (of course C++ doesn't support this via "new ValueIDNum[MaxNumBlocks][NumLocs]" (can't have multiple dynamic dimensions) - but a small wrapper type could allocate a flat array and do the x * width + y indexing via operator overloading)) | |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
925–927 | Looks like this adds an extra level of indirection that's not needed? FuncValueTable itself could be null (since it's a unique_ptr) rather than needing a pointer to it that is null? |
Jeremy's journey:
- Orlando: "This allocation/free code looks sketchy now,"
- dblaikie: "You should use std::unique_ptr!,"
- asan buildbots: "YOU SHOULD REALLY USE STD::UNIQUE_PTR"
Rebased onto main -- I cut out some dead code paths which now don't need to be updated, removed some un-necessary indirection, and some un-necessary calls to reset() that will now happen automagically due to unique_ptr-ness. I've also updated the unit tests, a purely mechanical change that alters what allocate the value tables.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
3159–3166 | That'd certainly be quicker -- however then it'd be more difficult to reduce peak memory consumption, as the whole table could only be freed in one go. There's probably a better representation of this information than "big tables in memory", it's not something I've gone near optimising for yet. | |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
925–927 | This is true, unfortunately it runs into some technical debt -- in an early phase, InstrRefBasedLDV steps through all instructions but it's too early to know how many registers/slots are going to be tracked, so these value tables aren't allocated yet. That's the only reason why these variables are pointers, so that on the early phase the tables can be optional. The root cause is that the early phase shares code with later phases: something that can be fixed, but as part of a larger refactor. |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
3257–3258 | What did you think of this (for depthFirstVLocAndEmit since emitLocations no longer exists)? |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
925–927 | Sorry, I'm not following. If you remove the extra indirection - concretely, what breaks? The FuncValueTable type is std::unique_ptr, so it can be null when the tables aren't allocated yet, right? I guess maybe because these are being passed by non-const ref, so they can't have an rvalue as a default argument? They could be passed by const ref instead (since unique_ptr has shallow const). That'd allow the nullptr default argument value. Though, having tried that manually/locally, and made that compile - maybe going a step further: Pass this as ValueTable* (pointer to the unique_ptr's buffer) since the ownership isn't relevant to the callees here, they just care about a buffer of ValueTables? (call site would have to use X.get() rather than the current &X - so it doesn't make the call site tidier, but does remove the extra indirection) |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
925–927 |
/me scratches head -- apparently I'd never considered unique_ptr acting like a ... normal pointer, which can be null. Yes, that'd work, I'll revise it in. |
Switch to passing an empty unique_ptr for value table arguments, instead of an optional raw pointer to a unique_ptr. This avoids un-necessary raw pointer usage.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp | ||
---|---|---|
3257–3258 | This is a possibility -- however this will only automatically free the outer dimension, and it'll change deallocation from happening when ExtendRanges returns, to when depthFirstVLocalAndEmit returns, which isn't substantial period of time. IMO, YMMV, having the outer dimension of value tables allocated/freed by ExtendRanges and always passed by reference to everything else, is cleaner to my mind that passing ownership in so that it can be freed. (I don't think it really matters one way or the other). |
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
925–927 | Thoughts on lowering this further, to ValueTable*? (since the clients know the length (unique_ptr doesn't carry the length anyway) & don't need to change the ownership/null out the original unique_ptr, etc) If it is going to stay as unique_ptr/FuncValueTable - should probably add const on these, since the unique_ptrs themselves aren't changing, only the things they point to, and unique_ptr doesn't have deep const, so it's sort of misleading/makes the reader think maybe the unique_ptr will change? |
clang-format and downgrade arguments to "process" to being ValueTable pointers -- no need to pass in the whole UniquePtr. Add const too.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h | ||
---|---|---|
925–927 | Done and done; the values are only read, so I've both made it ValueTable* and const. |
Looks like this adds an extra level of indirection that's not needed? FuncValueTable itself could be null (since it's a unique_ptr) rather than needing a pointer to it that is null?