This is an archive of the discontinued LLVM Phabricator instance.

Use SmallPtrSet explicitly for SmallSets with pointer types (NFC).
ClosedPublic

Authored by fhahn on Jun 6 2018, 10:24 AM.

Details

Summary

Currently SmallSet<PointerTy> inherits from SmallPtrSet<PointerTy>. This
patch replaces such types with SmallPtrSet, because IMO it is slightly
clearer and allows us to get rid of unnecessarily including SmallSet.h

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jun 6 2018, 10:24 AM

Seems OK to me - but not necessarily a strong motivation. Is the SmallPtr header dependency a significant impact? Would it be easier for to go the other way & normalize/standardize on SmallSet everywhere, if it reasonably does the right thing for pointers?

SmallSet<T*> has a template specialization that just inherits from SmallPtrSet.

As to the question about giong the other direction and using SmallSet everywehre. The real SmallSet class doesn't implement begin/end while SmallPtrSet does so we shouldn't convert everything that direction.

SmallSet<T*> has a template specialization that just inherits from SmallPtrSet.

*nod* So my question was "why ever write SmallPtrSet, why not just write SmallSet<T*> everywhere"?

As to the question about giong the other direction and using SmallSet everywehre. The real SmallSet class doesn't implement begin/end while SmallPtrSet does so we shouldn't convert everything that direction.

Oh, that's curious - though I'm going to guess that there's at least some uses that don't distinguish between how they name the type when they use begin/end, they probably just use it and it works.

In fact there is at least one place that iterates while using the SmallSet specialization on a pointer. It's DAGCombiner::MatchLoadCombine. Why is that not changed by this patch?

fhahn added a comment.Jun 7 2018, 3:43 PM

Yeah, I am not really sure there is a strong reason to change it. I just went for SmallPtrSet, because it is much wider used than SmallSet. And using SmallPtrSetImpl<> & on a SmallSet looks suprising.

dblaikie accepted this revision.Jun 8 2018, 11:17 AM

Seems OK to me (:

This revision is now accepted and ready to land.Jun 8 2018, 11:17 AM
fhahn updated this revision to Diff 150910.Jun 12 2018, 2:49 AM

rebased

This revision was automatically updated to reflect the committed changes.