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

Thu Jul 2 02:02:38 PDT 2020

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://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?

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.

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