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

Robinson, Paul via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 6 13:44:58 PDT 2020



> -----Original Message-----
> From: Simon Pilgrim <llvm-dev at redking.me.uk>
> Sent: Thursday, July 2, 2020 5:03 AM
> To: David Blaikie <dblaikie at gmail.com>; Duncan Exon Smith
> <dexonsmith at apple.com>
> Cc: Robinson, Paul <paul.robinson at sony.com>; Adrian Prantl
> <aprantl at apple.com>; Jonas Devlieghere <jdevlieghere at apple.com>; LLVM
> Commits <llvm-commits at lists.llvm.org>
> Subject: Re: [llvm] 0ae989a - Pass DebugLoc::appendInlinedAt DebugLoc arg
> by const reference not value.
> 
> 
> On 01/07/2020 21:30, David Blaikie wrote:
> > 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://urldefense.com/v3/__https://github.com/llvm/llvm-
> __;!!JmoZiZGBv3RvKRSx!vlKxX5OIOFP4SG6loHuSEPZzRS2Bl0QDjizVpE1VQeKgMjlz124N
> 6khP6tgRUuwnKQ$
> >>>>> project/commit/0ae989a1fede0e512e2bfd57b328aad6c1920329
> >>>>> DIFF: https://urldefense.com/v3/__https://github.com/llvm/llvm-
> __;!!JmoZiZGBv3RvKRSx!vlKxX5OIOFP4SG6loHuSEPZzRS2Bl0QDjizVpE1VQeKgMjlz124N
> 6khP6tgRUuwnKQ$
> >>>>> 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://urldefense.com/v3/__https://llvm.org/doxygen/Metadata_8cpp_source.
> html*l00153__;Iw!!JmoZiZGBv3RvKRSx!vlKxX5OIOFP4SG6loHuSEPZzRS2Bl0QDjizVpE1
> VQeKgMjlz124N6khP6tj7znUhxw$  - 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://urldefense.com/v3/__https://github.com/llvm/llvm-
> project/commit/5bf8fef58013e2c97180236fa6973faa40435d5f__;!!JmoZiZGBv3RvKR
> Sx!vlKxX5OIOFP4SG6loHuSEPZzRS2Bl0QDjizVpE1VQeKgMjlz124N6khP6tgjciS1Uw$
> >>> )
> >>>
> >>> 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?
> 
> Urgh, I didn't expect this patch to snowball, I'm sorry for pushing
> outstanding work up people's priority list...
> 
> Currently most places that a DebugLoc is passed by value is in
> constructors where its std::move()'d into place, as one could probably
> expect for a wrapped ptr style type.
> 
> There are a few other pass by value uses that seem to have snuck in
> though - AArch64InstrInfo::copyGPRRegTuple for instance (even though an
> equivalent use immediately below in AArch64InstrInfo::copyPhysReg uses
> const ref...) - and basically anywhere else where its only used
> immediately in BuildMI (etc.) calls.

Don't fret, Simon.  It's good for some of us to be internalizing this
"DebugLoc isn't for pass-by-value anymore" idea.  Like I said up front,
it used to be cheap, and now we're getting the idea that Duncan's
valuable work actually changed that.  Totally worthwhile.
You should definitely keep going with these tidy-up changes.
--paulr

> 
> >
> >>> 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://urldefense.com/v3/__https://lists.llvm.org/cgi-
> bin/mailman/listinfo/llvm-
> commits__;!!JmoZiZGBv3RvKRSx!vlKxX5OIOFP4SG6loHuSEPZzRS2Bl0QDjizVpE1VQeKgM
> jlz124N6khP6tiWGkvNjQ$


More information about the llvm-commits mailing list