[llvm] 0ae989a - Pass DebugLoc::appendInlinedAt DebugLoc arg by const reference not value.

[David Blaikie via llvm-commits]

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.

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.

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.

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