[LLVMdev] [llvm-commits] Review Request: Use SmallPtrSetImpl instead of SmallPtrSet in funciton IVUsers::AddUsersIfInteresting

[Andrew Trick]

On Mar 16, 2012, at 6:18 PM, Hongbin Zheng <etherzhhb at gmail.com> wrote:

> hi,
> 
> 
> On Sat, Mar 17, 2012 at 2:11 AM, Andrew Trick <atrick at apple.com> wrote:
> 
>> Yep. I normally do that. I was under some strange impression last night that SmallPtrSetImpl wasn't a template.
> The patch is incorrect because the SmallPtrSetImpl is neither a
> template nor has an "insert" function...
> 
> After a detailed look at the header of SmallPtrSet, I found that the
> SmallPtrSetImpl has an "insert_imp" function which accepts void
> pointer as argument, and the "insert" function in SmallPtrSet simply
> cast the incoming pointer to void* by the
> "PtrTraits::getAsVoidPointer" function and pass the void pointer to
> insert_imp.
> 
> So I wonder can we make SmallPtrSetImpl become a template just like
> SmallVectorImpl? After that we can move insert/erase/count from
> SmallPtrSet to SmallPtrSetImpl, and users can pass a
> SmallPtrSetImpl<T> as argument instead of pass something like
> SmallPtrSet<T, 100>.

I would personally like that. I think it was done this way to avoid code bloat. But as long as you keep the void* implementation in a nontemplate class (SmallPtrSetBase?), then I don't see a problem. You would effectively be moving some template instantiations from SmallPtrSet<T,size> into SmallPtrSetImpl<T>, which could be an improvement.

>> Please check in, and you can simultaneously fix the polly branch.
> Temporary fixed by passing a dummy set.
>> 
>> Incidentally, the only reason I didn't hide the SmallPtrSet argument behind the API is that all the callers outside IVUsers will be removed from the codebase soon.
> So which function is supposed to call by outside caller?

AddUsersIfInteresting is the correct function to call to update IVUsers. But soon LSR will be the only client of IVUsers, and IndVarSimplify won't need to update IVUsers at all.

-Andy