[lld] r210008 - Simplify markLive().

Rui Ueyama ruiu at google.com
Mon Jun 2 21:40:32 PDT 2014


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.

-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/310db6ee/attachment.html>


More information about the llvm-commits mailing list