[PATCH] D11345: ignore duplicate divisor uses when transforming into reciprocal multiplies (PR24141)

[hfinkel at anl.gov]

hfinkel added a comment.

In http://reviews.llvm.org/D11345#208773, @spatel wrote:

> In http://reviews.llvm.org/D11345#208610, @chandlerc wrote:
>
> > In http://reviews.llvm.org/D11345#208493, @spatel wrote:
> >
> > > Patch updated:
> > >  I just discovered 'SmallPtrSet'. 
> > >  Looks like it was designed exactly for this case, so let's use it here instead of a vector. Then, we don't have to use std::find() for dup checking.
> >
> >
> > Egads no, that makes the iteration order unstable.
> >
> > This whole thing seems wrongly formulated. Why are we walking the *entire* users list prior to finding out whether there are few enough users? I feel like the TLI interface should say how many can be handled rather than accepting a number we want to handle.
> >
> > And then the code here should be to insert into a SetVector until we exceed that threshold.
>
>
> I'm not following. We're asking the TLI if we have exceeded a minimum threshold, not a maximum. If we exceed the threshold, we want to transform all divisions that have this divisor...so we always have to walk all users?


Part of the issue here is that we might walk all of the users even though TLI.combineRepeatedFPDivisors will return false for any size. This is wasteful. We should just have TLI.combineRepeatedFPDivisors return the size threshold so that we can avoid walking if there is no point (and return false if no combination is ever desired).

Once we know that we're looking for a certain number of users, we can then see if we have enough. As Chandler says, you should use a SetVector so that the iteration order is stable.

Chandler, to be clear, we're not finding out if there are few enough users, we're walking to find out if there are enough (and, if there are, we need to iterate over all of them anyway).


http://reviews.llvm.org/D11345