<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 31, 2015 at 6:27 AM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Jul 29, 2015 at 8:40 PM, Puyan Lotfi <<a href="mailto:plotfi@apple.com">plotfi@apple.com</a>> wrote:<br>
> Justin, Aaron:<br>
><br>
> These are the revised patches.<br>
> I have tried to address all of your feedback with the exception of the naming of the ADT.<br>
> I was thinking, if I did rename it I could go with SortedVectorSet or SortedSet.<br>
><br>
> Anyways, I have added:<br>
><br>
> - Better documentation changes that try to be more clear on how and when to use SortedVector.<br>
> - Better comments.<br>
> - Removed erase().<br>
> - Replaced reset with clear.<br>
> - Renamed insert to push_back<br>
> - Added unit testing.<br>
><br>
> Hope this is enough to put my patch back in; please let me know if I missed anything.<br>
<br>
Comments below. Btw, I think this discussion should be on its own<br>
thread instead of attached to a commit revert message. I suspect there<br>
are others in the community who have opinions on this ADT, but are<br>
unaware of the discussion due to context. I've taken the liberty of<br>
starting a new email thread for this, and included a few usual<br>
suspects who review ADTs.<br>
<br>
For anyone joining in the middle, this is a discussion of an ADT Puyan<br>
proposed in:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/288930.html" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/288930.html</a><br>
<br>
That was commit (and eventually reverted) in:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150727/289945.html" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150727/289945.html</a><br>
<br>
> diff --git a/include/llvm/ADT/SortedVector.h b/include/llvm/ADT/SortedVector.h<br>
> new file mode 100644<br>
> index 0000000..ddafd6b<br>
> --- /dev/null<br>
> +++ b/include/llvm/ADT/SortedVector.h<br>
> @@ -0,0 +1,137 @@<br>
> +//===-- llvm/ADT/SortedVector.h ---------------------------------*- C++ -*-===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#ifndef LLVM_ADT_SORTEDVECTOR_H<br>
> +#define LLVM_ADT_SORTEDVECTOR_H<br>
> +<br>
> +#include <vector><br>
> +#include <cassert><br>
> +#include <functional><br>
> +<br>
> +namespace llvm {<br>
> +<br>
> +/// \brief A vector with minimal append overhead that enforces a sorted<br>
> +/// ordering of unique elements prior to access. The most common use case<br>
> +/// is to push_back multiple Entries and call sortUnique afterward.<br>
<br>
I am wondering why this should be an ADT instead of one-off<br>
functionality. I see that you're going to use it for<br>
MachineBasicBlock, but are there other use cases as well? The kind of<br>
functionality that this class provides is covered by basic STL<br>
mechanisms, and so I am hesitant to support it as an ADT for all of<br>
our users unless this is something we expect will be heavily used (or<br>
is heavily customized to support toolchain needs).<br>
<br></blockquote><div><br></div><div>Originally I had added a patch to improve compile time in VirtRegRewriter, and me and Quentin had noticed that these LiveIns set in MachineBasicBlock</div><div>we not actually in sorted order in all cases. We thought this could be a good way to maintain the ordering for the LiveIns set with low overhead since they were mostly sorted as a result of how they were being inserted into. Does a new ADT really need more than one client out the gate? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also, the comments do not specify what ordering it is sorted by (we<br>
can presume from the std::less, but it would be good to be explicit<br>
since this is user-facing documentation).<br></blockquote><div><br></div><div>Noted. I can add that comment.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +template<typename T, typename CMP = std::less<T>><br>
> +class SortedVector {<br>
> +private:<br>
> + typedef T value_type;<br>
> + typedef typename std::vector<value_type> vector_type;<br>
> + typedef typename vector_type::const_iterator const_iterator;<br>
> + typedef typename vector_type::iterator iterator;<br>
> + typedef typename vector_type::size_type size_type;<br>
<br>
These typedefs should be public, aside from vector_type.<br>
<br></blockquote><div><br></div><div>I must have mistaken the previous feedback, and thought you wanted them all private. Sorry. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Missing typedefs for difference_type, reference and const_reference.<br>
<br></blockquote><div><br></div><div>I didn't see any of this in say, UniqueVector. But I see it they are in SetVector. Yeah, I can certainly go add those. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + vector_type Vector;<br>
> + bool IsSorted = true;<br>
> +<br>
> + void AssertIsSorted() const {<br>
> + assert(IsSorted && "Unsorted SortedVector access; call sortUnique prior.");<br>
> + }<br>
> +<br>
> +public:<br>
> + /// \brief Appends Entry to the end of Vector; avoids appending duplicate<br>
> + /// entries. If appending Entry causes Vector to be unsorted, then sortUnique<br>
> + /// must be called prior to access.<br>
<br>
The public comments are still discussing private implementation<br>
details like Vector.<br>
<br></blockquote><div><br></div><div>So I shouldn't mention at all that internally this ADT contains a std::vector?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
How is the user of this function supposed to know when appending the<br>
entry causes the vector to be unsorted?<br>
<br></blockquote><div><br></div><div>The only way they know they are keeping the vector sorted is if they can guarantee that they had inserted every element in whatever sorted order that they specified in the CMP template parameter. Otherwise they should know to call sortUnique.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + void push_back(const value_type &Entry) {<br>
<br>
Should be using the typedef for const_reference.<br>
<br>
> + if (!Vector.size())<br>
<br>
Vector.empty() is more clear.<br>
<br></blockquote><div><br></div><div>Both noted.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + Vector.push_back(Entry);<br>
<br>
After pushing back in an empty vector, why are you relying on the<br>
std::equal_to to perform the return?<br>
<br></blockquote><div><br></div><div>If the Vector is empty I add the Entry no matter what.</div><div>Since it is the first Entry, it is sorted and I can return.</div><div>Entry is equal to itself, so it just returns. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + // If Entry is a duplicate of the previous Entry we skip.<br>
> + if (std::equal_to<value_type>()(Vector.back(), Entry))<br>
> + return;<br>
<br>
Why is this forcing use of std::equal_to, but we can specialize on<br>
something other than std::less? If someone is implementing a custom<br>
std::less implementation, it's very likely they would want a custom<br>
std::equal_to used as well.<br>
<br></blockquote><div><br></div><div>Make sense, I think could be added as a template parameter.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + IsSorted &= (CMP()(Vector.back(), Entry));<br>
> + Vector.push_back(Entry);<br>
> + }<br>
> +<br>
> + // \brief Sorts and uniques Vector.<br>
> + void sortUnique() {<br>
> + if (IsSorted)<br>
> + return;<br>
<br>
This still isn't correct. Consider code like:<br>
<br>
SortedVector<int> V;<br>
V.push_back(1);<br>
V.push_back(2);<br>
V.push_back(3);<br>
*V.begin() = 3;<br>
<br>
Not only is the vector no longer sorted (but it will claim that it is<br>
because IsSorted will be true), it's also not unique. We then fail to<br>
sort or unique the backing store.<br>
<br>
In order for this ADT to be viable, the underlying vector elements<br>
must be const so that you can never mutate their values through the<br>
ADT interface. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +<br>
> + std::sort(Vector.begin(), Vector.end());<br>
> + Vector.erase(std::unique(Vector.begin(), Vector.end()), Vector.end());<br>
<br>
Neither of these STL functions are using the custom comparator.<br>
Instead they use operator< on the object type, which is inconsistent.<br>
Should pass in CMP() to std::sort(), and should pass in the template<br>
parameter for std::equal_to for std::unique().<br>
<br>
> + IsSorted = true;<br>
> + }<br>
> +<br>
> + /// \brief Tells if Entry is in Vector without relying on sorted-uniqueness.<br>
> + bool has(value_type Entry) const {<br>
<br>
This should be using const_reference instead of forcing a by-value copy.<br>
<br>
> + if (IsSorted)<br>
> + return std::binary_search(Vector.begin(), Vector.end(), Entry);<br>
<br>
This function is using operator< to perform the comparisons instead of<br>
using CMP().<br>
<br>
> +<br>
> + return std::find(Vector.begin(), Vector.end(), Entry) != Vector.end();<br>
<br>
This function is using operator== to perform the comparisons instead<br>
of using a template parameter for std::equal_to.<br>
<br></blockquote><div><br></div><div>I can provide another revised patch with these feedback in mind.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + }<br>
<br>
I find it really strange that so many member functions call<br>
AssertIsSorted(), but the only truly interesting part of the interface<br>
does not. Why is it valid for you to call has() without using a sorted<br>
vector? This means the complexity of calling this function is either<br>
O(N) or O(log N), and you have no way of knowing which you'll get<br>
unless you call sortUnique() first.<br>
<br>
> +<br></blockquote><div><br></div><div>There are places that call MachineBasicBlock::isLiveIn. They just need to know if something is in the set or not.</div><div>I thought this would be a way to migrate existing code that checked if the entry was in the set before appending to it without changing a lot of the client code just yet.</div><div><br></div><div>Me and Quentin actually discussed this in the original feedback for the patch.</div><div>We were planning to add a heuristic to decide after a certain number of calls to has whether to sort. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + /// \brief Returns a reference to the Entry with the specified index.<br>
> + const value_type &operator[](unsigned index) const {<br>
<br>
This should be returning a const_reference and taking a size_type.<br>
<br>
> + assert(index < size() && "SortedVector index is out of range!");<br>
> + AssertIsSorted();<br>
> + return Vector[index];<br>
> + }<br>
> +<br>
> + /// \brief returns an iterator to the start of vector. Asserts if vector is<br>
> + /// not sorted and uniqued.<br>
> + iterator begin() {<br>
> + AssertIsSorted();<br>
> + return Vector.begin();<br>
> + }<br>
> +<br>
> + /// \brief Returns a const_iterator to the start of Vector. Asserts if<br>
> + /// Vector is not sorted and uniqued.<br>
> + const_iterator begin() const {<br>
> + AssertIsSorted();<br>
> + return Vector.begin();<br>
> + }<br>
> +<br>
> + /// \brief returns an iterator to the end of vector. Asserts if vector is<br>
> + /// not sorted and uniqued.<br>
> + iterator end() {<br>
> + AssertIsSorted();<br>
> + return Vector.end();<br>
> + }<br>
> +<br>
> + /// \brief Returns a const_iterator to the end of Vector. Asserts if<br>
> + /// Vector is not sorted and uniqued.<br>
> + const_iterator end() const {<br>
> + AssertIsSorted();<br>
> + return Vector.end();<br>
> + }<br>
> +<br>
> + /// \brief Erases Vector at position. Asserts if Vector not sorted and<br>
> + /// uniqued.<br>
> + iterator erase(iterator position) {<br>
> + AssertIsSorted();<br>
<br>
A vector iterator is a direct index into the vector; why is this<br>
assertion required for erasing an arbitrary element?<br></blockquote><div><br></div><div>Ah yeah, this should be taken out.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + return Vector.erase(position);<br>
> + }<br>
> +<br>
> + /// \brief Returns the total number of entries in Vector. Asserts if Vector<br>
> + /// not sorted and uniqued.<br>
> + size_type size() const {<br>
> + AssertIsSorted();<br>
<br>
Why is this assertion required for testing the size?<br>
<br></blockquote><div><br></div><div>My thinking was, that the size of the number of meaningful entries in the set would change when it gets uniqued so calling size before calling sortUnique not correct.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> + return Vector.size();<br>
> + }<br>
> +<br>
> + /// \brief Returns true if Vector is empty.<br>
> + bool empty() const {<br>
> + return Vector.empty();<br>
> + }<br>
> +<br>
> + /// \brief Clears all Entries in Vector.<br>
> + void clear() {<br>
> + IsSorted = true;<br>
> + Vector.clear();<br>
> + }<br>
> +};<br>
<br>
There is a lot of functionality missing from this interface that are<br>
concerns. For instance, there's no rvalue references, no constructors,<br>
pop_back, rbegin()/rend(), etc. This seems like a very limited<br>
interface in some ways, while way too permissive of an interface in<br>
other ways.<br></blockquote><div><br></div><div>That can be said for other ADTs as well, and I didn't know these were requirements.</div><div>Do I need to have everyone of these standard functions out the gate just to add a new ADT?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Even if you expanded the interface to cover more of the useful cases<br>
and fix the issues from the review, I am not certain this meets the<br>
bar to be an ADT. This, ultimately, is the stock STL vector that needs<br>
to remain ordered and unique. Why is an existing ordered, unique list<br>
(like std::set) insufficient?<br>
<br></blockquote><div><br></div><div>Because I think the implementation would be too messy in the code that used it, because I thought it would be better than having the same 3 lines of boilerplate sort-unique code copied and pasted, and also because I really thought this was a common enough pattern and that with the compile time benefit for the use case it was trying to address would be sufficient reason to add this one ADT to llvm.</div><div><br></div><div>You mentioned unique list (I assume You meant UniqueVector?), I don't think UniqueVector would pass some of the requirements stated here because I don't think the interface has all the functionality you mentioned was missing in SortedVector.</div><div><br></div><div>Also, I don't think the compile time impact of using std::set or UniqueVector makes them a good candidate for replacing std::vector in MachineBasicBlock::LiveIns (both use a red black tree if I am not mistaken). </div><div><br></div><div>I can go and change MachineBasicBlock::LiveIns' iterators to just call sort on the std::vector it was using but I think it would really make that code less readable, and I think there are probably plenty of other places in LLVM that could use this (albeit very simple) ADT.</div><div><br></div><div>-PL</div><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
~Aaron<br>
<br>
> +<br>
> +} // End of namespace llvm<br>
> +<br>
> +#endif // LLVM_ADT_SORTEDVECTOR_H<br>
> +<br>
><br>
</blockquote></div><br></div></div>