Page MenuHomePhabricator

[Bug 21683 relative] - Refactor of SimpleDefinedAtom::sortReferences()
AbandonedPublic

Authored by grimar on Sep 21 2015, 8:07 AM.

Details

Reviewers
kledzik
ruiu
Summary

I spent some time on investigating https://llvm.org/bugs/show_bug.cgi?id=21683
It looks already fixed in r223528 which switched SimpleDefinedAtom to allocate its SimpleReferences using the BumpPtrAllocator.

The only thing that does not look good for me is that SimpleDefinedAtom::sortReferences()
method uses temp SmallVector for copying into and sorting the ilist content and after that recreates the list again.
Since r247978 introduced ilist::sort I suggest the patch to use it.

Anyways I believe bug 21683 can be closed with this patch or without as fixed.

Diff Detail

Event Timeline

grimar updated this revision to Diff 35256.Sep 21 2015, 8:07 AM
grimar retitled this revision from to [Bug 21683 relative] - Refactor of SimpleDefinedAtom::sortReferences().
grimar updated this object.
grimar added reviewers: ruiu, kledzik.
grimar set the repository for this revision to rL LLVM.
grimar added a project: lld.
grimar added subscribers: grimar, llvm-commits.
ruiu edited edge metadata.Sep 21 2015, 11:17 AM
ruiu added a subscriber: ruiu.

Is the indentation correct?

denis-protivensky added inline comments.
Simple.h
188

Next time, please, provide the context.

This is a specialization of ilist_sentinel_traits template, and by changing the signature you made the default method play instead of this one. It may be a luck that sorting below worked for you, but it most likely hurt the correctness.

grimar updated this revision to Diff 35365.Sep 22 2015, 5:13 AM
grimar edited edge metadata.
grimar removed rL LLVM as the repository for this revision.

Fixed:
Indentations (sorry used tabs instead of spaces before)
No more possible leak of the sentinel if allocator was assigned after creation of it without allocator.

grimar added inline comments.Sep 22 2015, 5:17 AM
Simple.h
188

No, I dont think so. llvm::ilist inherits from ilist_sentinel_traits so we are free to declare destroySentinel static or not depending on our needs. That is widely used in another llvm code. I removed static modifier because we have non-static member variable here.

lhames added a subscriber: lhames.Sep 23 2015, 12:37 PM

I would expect the cost of a linked-list sort to quickly exceed the cost of copying to/from the vector and sorting there. If that's the case this tidy-up may not be worth the performance hit?

grimar abandoned this revision.Sep 24 2015, 11:08 AM