[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