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

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>.
> 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?
>
> -Andy
best regards
ether

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