[PATCH] D146136: [llvm][CycleInfo] Quick look-up for block in cycle.

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added a comment.

In D146136#4201718 <https://reviews.llvm.org/D146136#4201718>, @sameerds wrote:

> In D146136#4201559 <https://reviews.llvm.org/D146136#4201559>, @foad wrote:
>
>> In D146136#4201439 <https://reviews.llvm.org/D146136#4201439>, @sameerds wrote:
>>
>>> In D146136#4201388 <https://reviews.llvm.org/D146136#4201388>, @foad wrote:
>>>
>>>> Patch seems fine to me, but I wonder if we need to get more eyes on your SetVector changes. I don't think there is a code owner for ADT. @dblaikie?
>>>
>>> One good thing is that this not change the way SetVector is currently used. If all the user wants is a vector and a set of the same type, then nothing changed for them. But it does allow a new use-case by giving enough rope. It's difficult to specify static constraints. Can a user have a vector of float and a set of float? Sure they can, as long as they understand the lossy hashing produced by silently converting the float to int when inserting in the set!
>>
>> My only very slight unease was about which methods of SetVector should take key_type and which should take value_type. Is it obvious in all cases? Is there a precedent in STL (or Boost) to guide us?
>
> I couldn't find a precedent in the release versions, at least. I don't know if there are contributing projects currently experimenting with this concept.
>
> But to me, it was reasonably straight-forward to reason about this change because the SetVector already distinguishes between the uses of value_type and key_type. And it matched my intuition that the key_type satisfies constraints required by the set, and so it is only used in methods that rely on the properties provided by the set. Thus only two methods take key_type as argument: contains() and count(). All the other methods use value_type and they only access the vector, relying on its sequential nature. Insertion depends on the set for uniquing, and that's the only place where value_type is implicitly converted to key_type. But the insertion itself takes value_type because its primary purpose is to append to the vector.
>
> Another way to look at the distinction is that the use of the set in any method is entirely an optimization, and hence key_type is used only where the optimization is explicitly opted into. If we look at it that way, then the hypothetical overload "contains(const value_type &V)" should do a linear search in the vector because that's what the programmer wanted when they used the value_type instead of the key_type.

IMO this change makes sense and is a good QoL improvement so I would tend to say it's fine as-is.

Though I would split this into two commits:

- First, commit the CycleInfo change, but use the const_cast and the current API (no SetVector change)
- Then, commit the QoL change to SetVector's API.

That way, if someone, someday says that this change is heresy, it can very easily be reverted without having to understand the rest of this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146136/new/

https://reviews.llvm.org/D146136