[lld] r210008 - Simplify markLive().

Rui Ueyama ruiu at google.com
Mon Jun 2 22:02:41 PDT 2014


On Mon, Jun 2, 2014 at 9:40 PM, Rui Ueyama <ruiu at google.com> wrote:

> On Mon, Jun 2, 2014 at 9:28 PM, Nick Kledzik <kledzik at apple.com> wrote:
>
>>
>> On Jun 2, 2014, at 5:57 PM, Rui Ueyama <ruiu at google.com> wrote:
>>
>> On Mon, Jun 2, 2014 at 2:02 PM, Nick Kledzik <kledzik at apple.com> wrote:
>>
>>>
>>> On Jun 2, 2014, at 1:06 AM, Rui Ueyama <ruiu at google.com> wrote:
>>> > Author: ruiu
>>> > Date: Mon Jun  2 03:06:57 2014
>>> > New Revision: 210008
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=210008&view=rev
>>> > Log:
>>> > Simplify markLive().
>>> >
>>> > Reference::target() never returns a nullptr, so NULL check
>>> > is not needed and is more harmful than doing nothing.
>>> > No functionality change.
>>> Reference::target() can return nullptr.  From the header:
>>>
>>>   /// If the reference is an edge to another Atom, then this returns the
>>>   /// other Atom.  Otherwise, it returns nullptr.
>>>   virtual const Atom *target() const = 0;
>>>
>>> As the comment for the Reference class explains, they can be used for
>>> carrying
>>> extra data about an atom in which case the target() part is unused.
>>
>>
>> Is that comment still relevant? There are other places that assumes
>> target() never returns nullptr, and all uses of References in LLD are to
>> represent edges in graph of Atoms. The name "reference" also implies that
>> it refers something.
>>
>> So, if we need an attribute that has no target, I feel like it should be
>> represented by something other than Reference.
>>
>> I think in a discussion long ago we realized that if an atom wants to use
>> a Reference for internal purposes (such “here is a constant island”), it
>> won’t be using the target part, so it can set the reference’s target to be
>> the atom itself.  Then we never have nullptr targets.
>>
>> So, the comments should be updated to remove mentioning that the target
>> can be nullptr.
>>
>> Given that the target cannot be nullptr, does it make sense to change it
>> from Atom* to Atom&  ?
>>
>
> Yes I think so. I'll change the return type if no one seems to have
> objections.
>

I tried that and found that in many places return value of target() is used
as a pointer, such as in the context of dyn_cast, so it looked like it
would end up with adding many "&"s at the calling sites of target() without
adding any values.

So I don't think that is actually a good idea. I'll rather just updating
the comment of target().

 -Nick
>>
>>
>>
>>
>>
>>> -Nick
>>>
>>>
>>> >
>>> > Modified:
>>> >    lld/trunk/include/lld/Core/Resolver.h
>>> >    lld/trunk/lib/Core/Resolver.cpp
>>> >
>>> > Modified: lld/trunk/include/lld/Core/Resolver.h
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Core/Resolver.h?rev=210008&r1=210007&r2=210008&view=diff
>>> >
>>> ==============================================================================
>>> > --- lld/trunk/include/lld/Core/Resolver.h (original)
>>> > +++ lld/trunk/include/lld/Core/Resolver.h Mon Jun  2 03:06:57 2014
>>> > @@ -67,7 +67,7 @@ private:
>>> >   void checkDylibSymbolCollisions();
>>> >   void forEachUndefines(bool searchForOverrides, UndefCallback
>>> callback);
>>> >
>>> > -  void markLive(const Atom &atom);
>>> > +  void markLive(const Atom *atom);
>>> >   void addAtoms(const std::vector<const DefinedAtom *>&);
>>> >
>>> >   class MergedFile : public MutableFile {
>>> >
>>> > Modified: lld/trunk/lib/Core/Resolver.cpp
>>> > URL:
>>> http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Core/Resolver.cpp?rev=210008&r1=210007&r2=210008&view=diff
>>> >
>>> ==============================================================================
>>> > --- lld/trunk/lib/Core/Resolver.cpp (original)
>>> > +++ lld/trunk/lib/Core/Resolver.cpp Mon Jun  2 03:06:57 2014
>>> > @@ -305,17 +305,16 @@ void Resolver::updateReferences() {
>>> > }
>>> >
>>> > // For dead code stripping, recursively mark atoms "live"
>>> > -void Resolver::markLive(const Atom &atom) {
>>> > +void Resolver::markLive(const Atom *atom) {
>>> >   // Mark the atom is live. If it's already marked live, then stop
>>> recursion.
>>> > -  auto exists = _liveAtoms.insert(&atom);
>>> > +  auto exists = _liveAtoms.insert(atom);
>>> >   if (!exists.second)
>>> >     return;
>>> >
>>> >   // Mark all atoms it references as live
>>> > -  if (const DefinedAtom *defAtom = dyn_cast<DefinedAtom>(&atom))
>>> > +  if (const DefinedAtom *defAtom = dyn_cast<DefinedAtom>(atom))
>>> >     for (const Reference *ref : *defAtom)
>>> > -      if (const Atom *target = ref->target())
>>> > -        markLive(*target);
>>> > +      markLive(ref->target());
>>> > }
>>> >
>>> > // remove all atoms not actually used
>>> > @@ -342,7 +341,7 @@ void Resolver::deadStripOptimize() {
>>> >
>>> >   // mark all roots as live, and recursively all atoms they reference
>>> >   for (const Atom *dsrAtom : _deadStripRoots)
>>> > -    markLive(*dsrAtom);
>>> > +    markLive(dsrAtom);
>>> >
>>> >   // now remove all non-live atoms from _atoms
>>> >   _atoms.erase(std::remove_if(_atoms.begin(), _atoms.end(), [&](const
>>> Atom *a) {
>>> >
>>> >
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at cs.uiuc.edu
>>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140602/d20dc8ee/attachment.html>


More information about the llvm-commits mailing list