[PATCH] D90884: [SmallVector] Add `SVec<T>` / `Vec<T>` convenience wrappers.

[Sean Silva via Phabricator via llvm-commits]

silvas marked an inline comment as done.
silvas added inline comments.


================
Comment at: llvm/docs/ProgrammersManual.rst:1517
+* ``Vec<T>`` should be used for vectors on the heap. This includes inside other
+  data structures, such as ``DenseMap<Value *, Vec<Value *>>``.
+
----------------
mehdi_amini wrote:
> I don't necessarily subscribe to the stack/heap difference.
> 
> If the SVec/Vec is part of a heap allocation, they retain their properties: `Vect` would always an **extra** heap allocation for the data while SVec keeps inlined storage in the original heap allocation (with all the benefits of SmallVector).
> 
> I see it similarly to trailing allocation in heap allocated object somehow.
> In general, heap or stack, SmallVector and Vec behaves differently in "move" vs "copy".
This is definitely a simplification, and these are just "guidelines". There are definitely good reasons to use SVec on the heap or Vec on the stack. I think this is the safest default advice we can give, especially since large SmallVector on the heap seems to be one of the main patterns that has been historically problematic.


================
Comment at: llvm/docs/ProgrammersManual.rst:1521
+:ref:`SmallVector <dss_smallvector>` section, this has advantages over
+``std::vector<T>``, even with 0 inline elements.
+
----------------
mehdi_amini wrote:
> Should we say that we always recommend `Vec<T>` instead of `std::vector`? Are there use-cases (other than invoking external API expressed with std::vector) to use it?
> 
> (actually line 1617 may make this redundant)
Yes, std::vector is sometimes needed, see my comment below.


================
Comment at: llvm/docs/ProgrammersManual.rst:1618
 is often a better option due to the advantages listed above.  std::vector is
 still useful when you need to store more than ``UINT32_MAX`` elements or when
 interfacing with code that expects vectors :).
----------------
mehdi_amini wrote:
> Is this comment still relevant? The header for SmallVector is adjusting its max size right?
It is still relevant. The [current logic](https://github.com/llvm/llvm-project/blob/d8437552205d5036f3746d760002fbb637f4018d/llvm/include/llvm/ADT/SmallVector.h#L93) will still in many cases only allow UINT32_MAX elements.

```
template <class T>
using SmallVectorSizeType =
    typename std::conditional<sizeof(T) < 4 && sizeof(void *) >= 8, uint64_t,
                              uint32_t>::type;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90884