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

Duncan Exon Smith via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 1 11:12:05 PDT 2020



> 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&`.

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

> 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*`.

> 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