[llvm] 0ae989a - Pass DebugLoc::appendInlinedAt DebugLoc arg by const reference not value.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 6 13:45:56 PDT 2020
On Mon, Jul 6, 2020 at 1:45 PM Robinson, Paul <paul.robinson at sony.com> wrote:
>
>
>
> > -----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.
yep, +1 to all that
> --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