ADT: Make SmallVector::set_size() private

[dexonsmith]

Summary

Stop allowing use of SmallVectorBase::set_size() outside of the
SmallVector implementation, which sets the size without calling
constructors or destructors. Most callers should probably just use
resize(). Or, if the new size is guaranteed to be <= size() then
the new-ish truncate() works too (and optimizes better).

Some callers want to avoid initializing memory before copying in values.
Before this commit, this depended on reserve() and set_size():

auto Size = V.size();
V.reserve(V.size() + NumNew);      // Reserve for the expected size.
NumNew = initialize(V.end(), ...); // Get number added.
V.set_size(V.size() + NumNew);     // Set size to match.

Such code should be updated to use resize_for_overwrite() and
truncate():

auto Size = V.size();
V.resize_for_overwrite(Size + NumNew);       // Resize to expected size.
NumNew = initialize(V.begin() + Size, ...)); // Get number added.
V.truncate(Size + NumNew);                   // Truncate to match.

The new pattern is safe even for non-trivial types, since
resize_for_overwrite() calls constructors and truncate() calls
destructors. For trivial types, it should optimize the same way as the
old pattern.

Downstream code adopting this new pattern to adapt to the disappearance
of set_size() should carefully audit uses of V between the resize
and the truncate:

• Change V.size() => Size.
• Change V.capacity() => V.size() (mostly).
• Change V.end() => V.begin() + Size.
• If V is an out-parameter, early returns need a V.truncate() or V.clear(). A scope exit is recommended.

[dexonsmith]

Re-uploading diff to trigger bots now that dependent patches are live.

I'm measuring compile-time perf at http://llvm-compile-time-tracker.com for the full patch set to confirm this doesn't regress anything. I'll report back once I see results.

[dblaikie]

Sounds good - if the only places we're trying to use unitiliazide values is where they're POD (can you confirm that's the case/maybe mention it in the commit message that the extra power was unneeded), then removing this more powerful version that could be used for non-POD uninitialized values seems helpful.

llvm/include/llvm/ADT/SmallVector.h

587–589

I think maybe this can be removed entirely - all the call sites use "this->set_size", so I think they'd find that in the base class?

[dexonsmith]

llvm/include/llvm/ADT/SmallVector.h

587–589

My goal was to downgrade from protected to private, since there's no reason to let subclasses of SmallVectorImpl access this either. WDYT? Not worth it? Better comment?

[dexonsmith]

In D115380#3180943, @dblaikie wrote:

Sounds good - if the only places we're trying to use unitiliazide values is where they're POD (can you confirm that's the case/maybe mention it in the commit message that the extra power was unneeded), then removing this more powerful version that could be used for non-POD uninitialized values seems helpful.

Right, that's the idea. The one place I found doing something non-POD was ASTMatchers / https://reviews.llvm.org/D115379. It was:

V.reserve(size);
for (I : 0..size)
  savePointer(new (V[I]) T());
V.set_size();

and I changed it to:

V.reserve(Size);
for (I : 0..Size)
  savePointer(&V.emplace_back());

which seems more clear to me.

I'll expand the commit message to discuss that.

[dblaikie]

llvm/include/llvm/ADT/SmallVector.h

587–589

ah - somewhere between "not worth it" and "if you like".

If you want to do this, though - maybe a "using SuperClass::set_size" would be workable/better than the wrapper function? (not 100% sure that will work, but I think it should)

[dexonsmith]

In D115380#3180829, @dexonsmith wrote:

Re-uploading diff to trigger bots now that dependent patches are live.

I'm measuring compile-time perf at http://llvm-compile-time-tracker.com for the full patch set to confirm this doesn't regress anything. I'll report back once I see results.

Results here:
http://llvm-compile-time-tracker.com/compare.php?from=36e7b8dd564b383d9b4ca38674dbed6337e642b3&to=d1af2835b46f1a84c28942dae10cb1761ad3a57b&stat=instructions
I'm happy with what's there so I'll continue pushing these in.

llvm/include/llvm/ADT/SmallVector.h

587–589

Yeah; I considered that but didn't try it since I also wasn't 100% sure it would work (and if it didn't I might not have found the unnecessary uses in SmallString). But it's cleaner -- don't even need a comment I think -- so I'll do it now.

[dexonsmith]

Updated patch uses using SuperClass::set_size per @dblaikie's suggestion.

(No need for fresh eyes; just posting to get bots to run again in case there's some other user I missed (just found https://reviews.llvm.org/D117073 in MLIR with a last-minute grep).)

[dexonsmith]

In D115380#3236175, @dexonsmith wrote:

Updated patch uses using SuperClass::set_size per @dblaikie's suggestion.

(No need for fresh eyes; just posting to get bots to run again in case there's some other user I missed (just found https://reviews.llvm.org/D117073 in MLIR with a last-minute grep).)

Bots seem happy enough (Windows has one test failure, but that seems spurious since this patch only changes visibility). I expect to push imminently... just building locally again. Already uncovered one new use from https://reviews.llvm.org/D112160, which landed yesterday (fix was trivial; pushed as 9b85d7e166ac51a1cea66129edbfbc83fc5332b3).

llvm/include/llvm/ADT/SmallVector.h

protected:
   void grow_pod(void *FirstEl, size_t MinSize, size_t TSize);
 
 public:
   size_t size() const { return Size; }
   size_t capacity() const { return Capacity; }
 
   LLVM_NODISCARD bool empty() const { return !Size; }
 
+protected:
   /// Set the array size to \p N, which the current array must have enough
   /// capacity for.
   ///
   /// This does not construct or destroy any elements in the vector.
-  ///
-  /// Clients can use this in conjunction with capacity() to write past the end
-  /// of the buffer when they know that more elements are available, and only
-  /// update the size later. This avoids the cost of value initializing elements
-  /// which will only be overwritten.
   void set_size(size_t N) {
     assert(N <= capacity());
     Size = N;
   }
 };
 
 template <class T>
 using SmallVectorSizeType =
public:
   }
 
   void clear() {
     this->destroy_range(this->begin(), this->end());
     this->Size = 0;
   }
 
 private:
+  // Hide set_size() to avoid misuse.
+  void set_size(size_type N) { SuperClass::set_size(N); }
+
   template <bool ForOverwrite> void resizeImpl(size_type N) {
     if (N == this->size())
       return;
 
     if (N < this->size()) {
       this->truncate(N);
       return;
     }