<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 18, 2015 at 5:53 PM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra">Meta comment, use Phabricator?</div><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Mar 18, 2015 at 5:36 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">This just makes SmallDenseSet a thing, with no actual implementation changes, just template parameter changes.<div><div><div><br></div></div><div>My C++ fu is weak on the "how do i make this happen front", so feel free to point out a better way to do this.</div></div></div></blockquote><div><br></div></span><div>The change is fine in and of itself. We should add a unittest so that we actually make sure this code works and is compiling on every compiler.</div><div><br></div><div>However...</div><span class=""><div><br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div>SmallDenseSet is being used in a soon-to-be submitted out of tree patch.</div></div></div></blockquote><div><br></div></span><div>I'm sad you're using DenseSet at all. Does SmallPtrSet not work?</div></div></div></div></blockquote><div>It was for a std::pair.</div><div>SmallSet was significantly slower (IE 5% of the time for this pass) for almost any number i tried.</div><div><br></div><div>Actually, I think i can rearchitect this to not use any set at all for the moment :)</div><div>So i'm going to bail on this patch until it becomes necessary, since it seems simple enough to readd later.</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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> The implementation of DenseSet is... pretty inefficient. We should really build a variant of this which is its own thing rather than being layered on top of DenseMap, and potentially layer DenseMap on top of the set as is usually done. I've not really rushed off to do this because I almost never find that DenseSet is really what I need. I almost always actually need a SmallPtrSet or a sorted and uniqued vector. Anyways, that doesn't mean we shouldn't make the existing code for SmallDenseSet work or delete it, and check in a unittest that covers it if the code is going to stay.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Chandler</div></font></span><div><div class="h5"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div>(changed parts were reformatted with clang-format)</div><div><div>---</div><div> include/llvm/ADT/DenseSet.h | 16 +++++++++++++---</div><div> 1 file changed, 13 insertions(+), 3 deletions(-)</div><div><br></div><div>diff --git a/include/llvm/ADT/DenseSet.h b/include/llvm/ADT/DenseSet.h</div><div>index d340240..aee825c8 100644</div><div>--- a/include/llvm/ADT/DenseSet.h</div><div>+++ b/include/llvm/ADT/DenseSet.h</div><div>@@ -35,10 +35,12 @@ public:</div><div> }</div><div><br></div><div> /// DenseSet - This implements a dense probed hash-table based set.</div><div>-template<typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT> ></div><div>+template <typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT>,</div><div>+ typename DenseMapT =</div><div>+ DenseMap<ValueT, detail::DenseSetEmpty, ValueInfoT,</div><div>+ detail::DenseSetPair<ValueT>>></div><div> class DenseSet {</div><div>- typedef DenseMap<ValueT, detail::DenseSetEmpty, ValueInfoT,</div><div>- detail::DenseSetPair<ValueT>> MapTy;</div><div>+ typedef DenseMapT MapTy;</div><div> static_assert(sizeof(typename MapTy::value_type) == sizeof(ValueT),</div><div> "DenseMap buckets unexpectedly large!");</div><div> MapTy TheMap;</div><div>@@ -157,6 +159,14 @@ public:</div><div> }</div><div> };</div><div><br></div><div>+/// SmallDenseSet - This is the SmallDenseMap version of DenseSet</div><div>+template <typename ValueT, typename ValueInfoT = DenseMapInfo<ValueT>,</div><div>+ unsigned InlineBuckets = 4></div><div>+using SmallDenseSet =</div><div>+ DenseSet<ValueT, ValueInfoT,</div><div>+ SmallDenseMap<ValueT, detail::DenseSetEmpty, InlineBuckets,</div><div>+ ValueInfoT, detail::DenseSetPair<ValueT>>>;</div><div>+</div><div> } // end namespace llvm</div><div><br></div><div> #endif</div><div>--</div><div>2.3.3</div></div></div></div></blockquote></div></div></div><br><br></div></div>
</blockquote></div><br></div></div>