[llvm] 0ae989a - Pass DebugLoc::appendInlinedAt DebugLoc arg by const reference not value.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 1 13:30:09 PDT 2020
On Wed, Jul 1, 2020 at 11:12 AM Duncan Exon Smith <dexonsmith at apple.com> wrote:
>
>
>
> > On 2020-Jul-01, at 10:01, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > On Wed, Jul 1, 2020 at 9:11 AM Robinson, Paul <paul.robinson at sony.com> wrote:
> >>
> >>
> >>
> >>> -----Original Message-----
> >>> From: llvm-commits <llvm-commits-bounces at lists.llvm.org> On Behalf Of
> >>> Simon Pilgrim via llvm-commits
> >>> Sent: Wednesday, July 1, 2020 11:39 AM
> >>> To: llvm-commits at lists.llvm.org
> >>> Subject: [llvm] 0ae989a - Pass DebugLoc::appendInlinedAt DebugLoc arg by
> >>> const reference not value.
> >>>
> >>>
> >>> Author: Simon Pilgrim
> >>> Date: 2020-07-01T16:38:51+01:00
> >>> New Revision: 0ae989a1fede0e512e2bfd57b328aad6c1920329
> >>>
> >>> URL: https://github.com/llvm/llvm-
> >>> project/commit/0ae989a1fede0e512e2bfd57b328aad6c1920329
> >>> DIFF: https://github.com/llvm/llvm-
> >>> project/commit/0ae989a1fede0e512e2bfd57b328aad6c1920329.diff
> >>>
> >>> LOG: Pass DebugLoc::appendInlinedAt DebugLoc arg by const reference not
> >>> value.
> >>>
> >>> Noticed by clang-tidy performance-unnecessary-value-param warning.
> >>
> >> Is it really a performance thing? Somehow I had it in my head that
> >> DebugLoc was deliberately lightweight to make it easy to pass by value;
> >> perhaps I'm thinking of something else, as I don't see a comment to
> >> that effect in DebugLoc.h, but still it doesn't look that heavy.
> >
> > That's my understanding too... though, looking at it.
>
> As David discovered, I changed that at some point. A simpler way to make the argument lightweight is to pass an `MDLocation*` or `MDLocation&`.
Looks about the same amount as simple as passing the DebugLoc by const
ref? I guess MDLocation isn't copyable, perhaps, so it's more obvious
there's no choice but to handle it by pointer or ref?
> > It seems this boils down to
> > https://llvm.org/doxygen/Metadata_8cpp_source.html#l00153 - so it's
> > essentially like a std::shared_ptr, doing ref counting.
>
> More like a TrackingVH. IIRC, there's no refcounting, it's just needed for RAUW when parsing IR.
Oh, looked at the code again - I see it's only ref counted for
replaceable metadata (then there's an "addRef" call) & something else
interesting that happens for "distinct" placeholder nodes too.
>
> > Seems this has been this way for a while (since 2014:
> > https://github.com/llvm/llvm-project/commit/5bf8fef58013e2c97180236fa6973faa40435d5f
> > )
> >
> > Duncan - any thoughts on this? I haven't looked more closely at the
> > patch yet to better understand the motivation for this particular part
> > of the change you made back then - but perhaps you remember/it's
> > obvious enough to you.
>
> [ I'm not sure I've been able to fully page this back in, but here's what I have... ]
>
> I think the main point of this part was to replace the table in the LLVMContext (see the change in DebugLoc.cpp) that was hard to reason about with the `MDLocation` class. IIRC, `DebugLoc` is used in places (like `Instruction`) where we need a tracking handle (for parsing IR), so it became heavy, but where you don't need a tracking handle you can use `MDLocation&` (or `MDLocation*`) directly.
>
> IMO, the `DebugLoc` class should just be deleted in favour of using `MDLocation` directly. I probably intended to circle back and do that. The generator APIs can be moved to `MDLocation`, which they're mostly just wrappers around anyway. The few places a tracking handle is needed should use the equivalent `TrackingMDRef<MDLocation>` and it's more obvious they're expensive to copy. The other sites can just use an `MDLocation&` or `MDLocation*`.
Sounds plausible/reasonable - perhaps you/someone could add a quick
FIXME to DebugLoc.h describing this deprecation/migration strategy?
>
> > Certainly is something to keep in mind - I wouldn't copy a
> > std::shared_ptr if I didn't need to, and I probably will make a point
> > of not copying DebugLocs needlessly either if this is the
> > correct/ongoing implementation.
> >
> > - Dave
> >
> >> --paulr
> >>
> >>>
> >>> Added:
> >>>
> >>>
> >>> Modified:
> >>> llvm/include/llvm/IR/DebugLoc.h
> >>> llvm/lib/IR/DebugLoc.cpp
> >>>
> >>> Removed:
> >>>
> >>>
> >>>
> >>> ##########################################################################
> >>> ######
> >>> diff --git a/llvm/include/llvm/IR/DebugLoc.h
> >>> b/llvm/include/llvm/IR/DebugLoc.h
> >>> index 780d17a33661..4914d733fe0d 100644
> >>> --- a/llvm/include/llvm/IR/DebugLoc.h
> >>> +++ b/llvm/include/llvm/IR/DebugLoc.h
> >>> @@ -85,7 +85,7 @@ namespace llvm {
> >>> /// the chain now is inlined-at the new call site.
> >>> /// \param InlinedAt The new outermost inlined-at in the chain.
> >>> /// \param ReplaceLast Replace the last location in the inlined-at
> >>> chain.
> >>> - static DebugLoc appendInlinedAt(DebugLoc DL, DILocation *InlinedAt,
> >>> + static DebugLoc appendInlinedAt(const DebugLoc &DL, DILocation
> >>> *InlinedAt,
> >>> LLVMContext &Ctx,
> >>> DenseMap<const MDNode *, MDNode *>
> >>> &Cache,
> >>> bool ReplaceLast = false);
> >>>
> >>> diff --git a/llvm/lib/IR/DebugLoc.cpp b/llvm/lib/IR/DebugLoc.cpp
> >>> index 14d1396f1543..e945cbcba782 100644
> >>> --- a/llvm/lib/IR/DebugLoc.cpp
> >>> +++ b/llvm/lib/IR/DebugLoc.cpp
> >>> @@ -79,7 +79,7 @@ DebugLoc DebugLoc::get(unsigned Line, unsigned Col,
> >>> const MDNode *Scope,
> >>> const_cast<MDNode *>(InlinedAt), ImplicitCode);
> >>> }
> >>>
> >>> -DebugLoc DebugLoc::appendInlinedAt(DebugLoc DL, DILocation *InlinedAt,
> >>> +DebugLoc DebugLoc::appendInlinedAt(const DebugLoc &DL, DILocation
> >>> *InlinedAt,
> >>> LLVMContext &Ctx,
> >>> DenseMap<const MDNode *, MDNode *>
> >>> &Cache,
> >>> bool ReplaceLast) {
> >>>
> >>>
> >>>
> >>> _______________________________________________
> >>> llvm-commits mailing list
> >>> llvm-commits at lists.llvm.org
> >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list