This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][InstrRef][NFC] Use unique_ptr instead of raw pointers for value tables
ClosedPublic

Authored by jmorse on Feb 2 2022, 3:52 AM.

Details

Summary

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.

Diff Detail

Event Timeline

jmorse created this revision.Feb 2 2022, 3:52 AM
jmorse requested review of this revision.Feb 2 2022, 3:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 3:52 AM

Nice. I have a few questions / nits but I'm a fan of the change.

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
3257–3258

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?

3258–3259

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?

3266–3267

You don't need to call reset since MOutLocs is scoped to this function (the unqiue_ptr's dtor will clean up for you).

dblaikie added inline comments.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
2981–2983

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)

3160–3167

prefer std::make_unique if possible

3160–3167

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?

jmorse updated this revision to Diff 405606.EditedFeb 3 2022, 6:04 AM

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.

jmorse marked 2 inline comments as done.Feb 3 2022, 6:14 AM
jmorse added inline comments.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
3160–3167

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.

Orlando added inline comments.Feb 3 2022, 6:43 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
3258–3259

What did you think of this (for depthFirstVLocAndEmit since emitLocations no longer exists)?

dblaikie added inline comments.Feb 4 2022, 5:31 PM
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)

jmorse added inline comments.Feb 8 2022, 9:45 AM
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.h
925–927

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?

/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.

jmorse updated this revision to Diff 410848.Feb 23 2022, 9:14 AM

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.

jmorse marked 2 inline comments as done.Feb 23 2022, 9:19 AM
jmorse added inline comments.
llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
3258–3259

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).

dblaikie added inline comments.Feb 23 2022, 9:24 AM
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?

jmorse updated this revision to Diff 411810.Feb 28 2022, 7:25 AM

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.

dblaikie accepted this revision.Feb 28 2022, 9:07 AM

Looks good on my side - might be worth checking with @Orlando too before submitting.

This revision is now accepted and ready to land.Feb 28 2022, 9:07 AM

Looks good on my side - might be worth checking with @Orlando too before submitting.

LGTM to me too, thanks for checking.

This revision was landed with ongoing or failed builds.Mar 1 2022, 4:50 AM
This revision was automatically updated to reflect the committed changes.