[mlir] Move the Operation OperandStorage to the first trailing object

[rriddle]

Summary

The main benefits of this change are faster access to operands
(no need to compute the offset, as it is now right after the
operation), simpler code(no need to manage a lot of the "is the
operand storage trailing" logic we had to before). The major
downside to this though, is that operand holding operations now
grow in size by 1 word (as no matter how we do this change, there
will need to be some additional book keeping).

[lattner]

nice, I'll take a look at this this weekend, thank you for sharing it!

[lattner]

Cool, thank you for this patch River. My ability to look at this was a little overoptimistic, but I applied this now and gave it a try, it shows pretty much exactly what I was expecting.

Before (total time on all threads + wall time):

194.4689 (100.0%)   72.7334 (100.0%)  Total

After:

188.3475 (100.0%)   70.4277 (100.0%)  Total

Which is about a 3% speedup.

[lattner]

That said, I'd recommend a minor change. Rearranging the fields in OperandStorage and making capacity be the compressed one instead of numOperands provides a further speedup:

176.2160 (100.0%)   67.5828 (100.0%)  Total

The field order looks like this:

/// The total capacity number of operands that the storage can hold.
unsigned capacity : 31;
/// A flag indicating if the operand storage was dynamically allocated, as
/// opposed to inlined into the owning operation.
unsigned isStorageDynamic : 1;
/// The number of operands within the storage.
unsigned numOperands;
/// A pointer to the operand storage.
OpOperand *operandStorage;

This benchmark is a macro benchmark running firtool on a large .fir design (parsing 589MB of .fir file, and emitting 307MB of verilog) on an 8 core intel MBP. The full timing output is:

===-------------------------------------------------------------------------===
                         ... Execution time report ...
===-------------------------------------------------------------------------===
  Total Execution Time: 67.5828 seconds

  ----User Time----  ----Wall Time----  ----Name----
    7.2946 (  4.1%)    7.2946 ( 10.8%)  FIR Parser
  120.3814 ( 68.3%)   44.2111 ( 65.4%)  'firrtl.circuit' Pipeline
   22.7650 ( 12.9%)    2.0119 (  3.0%)    'firrtl.module' Pipeline
   19.0680 ( 10.8%)    1.8196 (  2.7%)      CSE
    0.0207 (  0.0%)    0.0032 (  0.0%)        (A) DominanceInfo
    3.6656 (  2.1%)    0.3209 (  0.5%)      LowerCHIRRTL
    5.8990 (  3.3%)    5.8990 (  8.7%)    InferWidths
    3.1869 (  1.8%)    3.1869 (  4.7%)    InferResets
    0.3583 (  0.2%)    0.3583 (  0.5%)      (A) circt::firrtl::InstanceGraph
    0.3518 (  0.2%)    0.3518 (  0.5%)    PrefixModules
   11.5896 (  6.6%)   11.5896 ( 17.1%)    LowerFIRRTLTypes
   50.5189 ( 28.7%)    8.7349 ( 12.9%)    'firrtl.module' Pipeline
   19.0035 ( 10.8%)    3.6512 (  5.4%)      ExpandWhens
   31.4828 ( 17.9%)    5.0830 (  7.5%)      Canonicalizer
    1.2106 (  0.7%)    1.2106 (  1.8%)    Inliner
    7.1370 (  4.1%)    7.1370 ( 10.6%)    IMConstProp
    0.4026 (  0.2%)    0.4026 (  0.6%)      (A) circt::firrtl::InstanceGraph
    0.0018 (  0.0%)    0.0018 (  0.0%)    BlackBoxReader
   13.5857 (  7.7%)    2.1557 (  3.2%)    'firrtl.module' Pipeline
   13.5538 (  7.7%)    2.1522 (  3.2%)      Canonicalizer
    0.7672 (  0.4%)    0.7672 (  1.1%)    CreateSiFiveMetadata
    1.1527 (  0.7%)    1.1527 (  1.7%)    EmitOMIR
    0.3725 (  0.2%)    0.3725 (  0.6%)      (A) circt::firrtl::InstanceGraph
    3.7628 (  2.1%)    3.7628 (  5.6%)  LowerFIRRTLToHW
    0.6715 (  0.4%)    0.6715 (  1.0%)  HWMemSimImpl
    2.8777 (  1.6%)    2.8777 (  4.3%)  SVExtractTestCode
   34.3632 ( 19.5%)    3.5022 (  5.2%)  'hw.module' Pipeline
    0.6692 (  0.4%)    0.0587 (  0.1%)    HWCleanup
   11.2831 (  6.4%)    1.1087 (  1.6%)    CSE
    0.0390 (  0.0%)    0.0059 (  0.0%)      (A) DominanceInfo
   18.6762 ( 10.6%)    2.1384 (  3.2%)    Canonicalizer
    0.1391 (  0.1%)    0.0152 (  0.0%)    HWLegalizeModules
    3.3793 (  1.9%)    0.2665 (  0.4%)    PrettifyVerilog
    5.1930 (  2.9%)    5.1930 (  7.7%)  ExportVerilog
    0.0595 (  0.0%)    0.0595 (  0.1%)  Rest
  176.2160 (100.0%)   67.5828 (100.0%)  Total

I think we should take this patch, with the revisions to OperandStorage.

[lattner]

FWIW, I also just experimented with removing OperandStorage from the TrailingResults and make it be an unconditional member of Operation. This simplifies the generated machine code by eliminating "hasOperandStorage" and all the branches on it.

Doing this shows an extremely tiny speedup (67.4075s) but since it increases memory usage for commonly used operations, I don't think we should eliminate it.

[lattner]

mlir/include/mlir/IR/OperationSupport.h

567

Please change these fields to:

/// The total capacity number of operands that the storage can hold.
unsigned capacity : 31;
/// A flag indicating if the operand storage was dynamically allocated, as
/// opposed to inlined into the owning operation.
unsigned isStorageDynamic : 1;
/// The number of operands within the storage.
unsigned numOperands;
/// A pointer to the operand storage.
OpOperand *operandStorage;

This shows a further speedup on top of this patch because "numOperands" is frequently accessed and capacity is not.

[lattner]

Actually, wait, this isn't done yet: the OperandRange::OperandRange(Operation*) convenience constructor is still 2% of the profile because it is out-of-line. Please remove it (from the .h file and the OperationSupport.cpp file) as it is trivial and only used in one place. Please change Operation::getOperands() to be defined as:

   /// Returns an iterator on the underlying Value's.
-  operand_range getOperands() { return operand_range(this); }
+  operand_range getOperands() {
+    return operand_range(getOpOperands().data(), getNumOperands());
+  }

or equivalent. This change further reduces time from 67.5s to 66.39s!

[rriddle]

resolve comments

[lattner]

mlir/include/mlir/IR/OperationSupport.h

864–865

Is this hunk related?

mlir/lib/IR/Operation.cpp

22

I think something unrelated snuck into this patch set.

651–652

FYI, This looks unrelated to the patch.

[rriddle]

mlir/lib/IR/Operation.cpp

651–652

I'm not seeing any of these hunks from my view, weird phab thing?

[lattner]

Yes, looks like it was showing me stuff that came in in your merge. The patch lgtm, thanks!

[rriddle]

mlir/lib/IR/Operation.cpp

651–652

Oh I see what it is. You are viewing the diff between patches, not base (I only ever look at the diff from base). I've rebased since the last patch.

[lattner]

woo hoo, great work River!

mlir/include/mlir/IR/Operation.h

 
 namespace mlir {
 /// Operation is a basic unit of execution within MLIR. Operations can
 /// be nested within `Region`s held by other operations effectively forming a
 /// tree. Child operations are organized into operation blocks represented by a
 /// 'Block' class.
 class alignas(8) Operation final
     : public llvm::ilist_node_with_parent<Operation, Block>,
-      private llvm::TrailingObjects<Operation, BlockOperand, Region,
-                                    detail::OperandStorage> {
+      private llvm::TrailingObjects<Operation, detail::OperandStorage,
+                                    BlockOperand, Region, OpOperand> {
 public:
   /// Create a new Operation with the specific fields.
   static Operation *create(Location location, OperationName name,
                            TypeRange resultTypes, ValueRange operands,
                            ArrayRef<NamedAttribute> attributes,
                            BlockRange successors, unsigned numRegions);
 
   /// Overload of create that takes an existing DictionaryAttr to avoid
private:
 
   // allow value to access the 'ResultStorage' methods.
   friend class Value;
 
   // allow ilist_node_with_parent to access the 'getParent' method.
   friend class llvm::ilist_node_with_parent<Operation, Block>;
 
   // This stuff is used by the TrailingObjects template.
-  friend llvm::TrailingObjects<Operation, BlockOperand, Region,
-                               detail::OperandStorage>;
+  friend llvm::TrailingObjects<Operation, detail::OperandStorage, BlockOperand,
+                               Region, OpOperand>;
+  size_t numTrailingObjects(OverloadToken<detail::OperandStorage>) const {
+    return hasOperandStorage ? 1 : 0;
+  }
   size_t numTrailingObjects(OverloadToken<BlockOperand>) const {
     return numSuccs;
   }
   size_t numTrailingObjects(OverloadToken<Region>) const { return numRegions; }
 };
 
 inline raw_ostream &operator<<(raw_ostream &os, Operation &op) {
   op.print(os, OpPrintingFlags().useLocalScope());

mlir/include/mlir/IR/OperationSupport.h

public:
   MLIRContext *getContext() const { return location->getContext(); }
 };
 
 //===----------------------------------------------------------------------===//
 // OperandStorage
 //===----------------------------------------------------------------------===//
 
 namespace detail {
-/// This class contains the information for a trailing operand storage.
-struct TrailingOperandStorage final
-    : public llvm::TrailingObjects<TrailingOperandStorage, OpOperand> {
-#if defined(BYTE_ORDER) && defined(BIG_ENDIAN) && (BYTE_ORDER == BIG_ENDIAN)
-  TrailingOperandStorage() : numOperands(0), capacity(0), reserved(0) {}
-#else
-  TrailingOperandStorage() : reserved(0), capacity(0), numOperands(0) {}
-#endif
-  ~TrailingOperandStorage() {
-    for (auto &operand : getOperands())
-      operand.~OpOperand();
-  }
-
-  /// Return the operands held by this storage.
-  MutableArrayRef<OpOperand> getOperands() {
-    return {getTrailingObjects<OpOperand>(), numOperands};
-  }
-
-#if defined(BYTE_ORDER) && defined(BIG_ENDIAN) && (BYTE_ORDER == BIG_ENDIAN)
-  /// The number of operands within the storage.
-  unsigned numOperands;
-  /// The total capacity number of operands that the storage can hold.
-  unsigned capacity : 31;
-  /// We reserve a range of bits for use by the operand storage.
-  unsigned reserved : 1;
-#else
-  /// We reserve a range of bits for use by the operand storage.
-  unsigned reserved : 1;
-  /// The total capacity number of operands that the storage can hold.
-  unsigned capacity : 31;
-  /// The number of operands within the storage.
-  unsigned numOperands;
-#endif
-};
-
 /// This class handles the management of operation operands. Operands are
 /// stored either in a trailing array, or a dynamically resizable vector.
-class OperandStorage final
-    : private llvm::TrailingObjects<OperandStorage, OpOperand> {
+class alignas(8) OperandStorage {
 public:
-  OperandStorage(Operation *owner, ValueRange values);
+  OperandStorage(Operation *owner, OpOperand *trailingOperands,
+                 ValueRange values);
   ~OperandStorage();
 
   /// Replace the operands contained in the storage with the ones provided in
   /// 'values'.
   void setOperands(Operation *owner, ValueRange values);
 
   /// Replace the operands beginning at 'start' and ending at 'start' + 'length'
   /// with the ones provided in 'operands'. 'operands' may be smaller or larger
   /// than the range pointed to by 'start'+'length'.
   void setOperands(Operation *owner, unsigned start, unsigned length,
                    ValueRange operands);
 
   /// Erase the operands held by the storage within the given range.
   void eraseOperands(unsigned start, unsigned length);
 
   /// Erase the operands held by the storage that have their corresponding bit
   /// set in `eraseIndices`.
   void eraseOperands(const llvm::BitVector &eraseIndices);
 
   /// Get the operation operands held by the storage.
-  MutableArrayRef<OpOperand> getOperands() {
-    return getStorage().getOperands();
-  }
+  MutableArrayRef<OpOperand> getOperands() { return {operandStorage, size()}; }
 
   /// Return the number of operands held in the storage.
-  unsigned size() { return getStorage().numOperands; }
-
-  /// Returns the additional size necessary for allocating this object.
-  static size_t additionalAllocSize(unsigned numOperands) {
-    return additionalSizeToAlloc<OpOperand>(numOperands);
-  }
+  unsigned size() { return numOperands; }
 
 private:
-  /// Pointer type traits for the storage pointer that ensures that we use the
-  /// lowest bit for the storage pointer.
-  struct StoragePointerLikeTypeTraits
-      : llvm::PointerLikeTypeTraits<TrailingOperandStorage *> {
-    static constexpr int NumLowBitsAvailable = 1;
-  };
-
   /// Resize the storage to the given size. Returns the array containing the new
   /// operands.
   MutableArrayRef<OpOperand> resize(Operation *owner, unsigned newSize);
 
-  /// Returns the current internal storage instance.
-  TrailingOperandStorage &getStorage() {
-    return LLVM_UNLIKELY(isDynamicStorage()) ? getDynamicStorage()
-                                             : getInlineStorage();
-  }
-
-  /// Returns the storage container if the storage is inline.
-  TrailingOperandStorage &getInlineStorage() {
-    assert(!isDynamicStorage() && "expected storage to be inline");
-    return inlineStorage;
-  }
-
-  /// Returns the storage container if this storage is dynamic.
-  TrailingOperandStorage &getDynamicStorage() {
-    assert(isDynamicStorage() && "expected dynamic storage");
-    return *dynamicStorage.getPointer();
-  }
-
-  /// Returns true if the storage is currently dynamic.
-  bool isDynamicStorage() const { return dynamicStorage.getInt(); }
-
-  /// The current representation of the storage. This is either a
-  /// InlineOperandStorage, or a pointer to a InlineOperandStorage.
-  union {
-    TrailingOperandStorage inlineStorage;
-    llvm::PointerIntPair<TrailingOperandStorage *, 1, bool,
-                         StoragePointerLikeTypeTraits>
-        dynamicStorage;
-  };
-
-  /// This stuff is used by the TrailingObjects template.
-  friend llvm::TrailingObjects<OperandStorage, OpOperand>;
+  /// The total capacity number of operands that the storage can hold.
+  unsigned capacity;
+  /// The number of operands within the storage.
+  unsigned numOperands : 31;
+  /// A flag indicating if the operand storage was dynamically allocated, as
+  /// opposed to inlined into the owning operation.
+  unsigned isStorageDynamic : 1;
+  /// A pointer to the operand storage.
+  OpOperand *operandStorage;
 };
 } // end namespace detail
 
 //===----------------------------------------------------------------------===//
 // OpPrintingFlags
 //===----------------------------------------------------------------------===//
 
 /// Set of flags used to control the behavior of the various IR print methods
public:
   use_iterator use_begin() const;
   use_iterator use_end() const;
 
   /// Returns true if no results in this range have uses.
   bool use_empty() const {
     return llvm::all_of(*this,
                         [](OpResult result) { return result.use_empty(); });
   }
 
   //===--------------------------------------------------------------------===//
   // Users
   //===--------------------------------------------------------------------===//
 
   using user_iterator = ValueUserIterator<use_iterator, OpOperand>;
   using user_range = iterator_range<user_iterator>;
 
   /// Returns a range of all users.
   user_range getUsers();

mlir/lib/IR/Operation.cpp

 #include "mlir/IR/PatternMatch.h"
 #include "mlir/IR/TypeUtilities.h"
 #include "mlir/Interfaces/FoldInterfaces.h"
 #include "llvm/ADT/StringExtras.h"
 #include <numeric>
 
 using namespace mlir;
 
 OpAsmParser::~OpAsmParser() {}
 
 //===----------------------------------------------------------------------===//
 // OperationName
 //===----------------------------------------------------------------------===//
 
 /// Form the OperationName for an op with the specified string.  This either is
 /// a reference to an AbstractOperation if one is known, or a uniqued Identifier
 /// if not.
if (operands.empty()) {
     if (const AbstractOperation *abstractOp = name.getAbstractOperation())
       needsOperandStorage = !abstractOp->hasTrait<OpTrait::ZeroOperands>();
   }
 
   // Compute the byte size for the operation and the operand storage. This takes
   // into account the size of the operation, its trailing objects, and its
   // prefixed objects.
   size_t byteSize =
-      totalSizeToAlloc<BlockOperand, Region, detail::OperandStorage>(
-          numSuccessors, numRegions, needsOperandStorage ? 1 : 0) +
-      detail::OperandStorage::additionalAllocSize(numOperands);
+      totalSizeToAlloc<detail::OperandStorage, BlockOperand, Region, OpOperand>(
+          needsOperandStorage ? 1 : 0, numSuccessors, numRegions, numOperands);
   size_t prefixByteSize = llvm::alignTo(
       Operation::prefixAllocSize(numTrailingResults, numInlineResults),
       alignof(Operation));
   char *mallocMem = reinterpret_cast<char *>(malloc(byteSize + prefixByteSize));
   void *rawMem = mallocMem + prefixByteSize;
 
   // Create the new Operation.
   Operation *op =
new (op->getOutOfLineOpResult(i))
         detail::OutOfLineOpResult(*resultTypeIt, i);
   }
 
   // Initialize the regions.
   for (unsigned i = 0; i != numRegions; ++i)
     new (&op->getRegion(i)) Region(op);
 
   // Initialize the operands.
-  if (needsOperandStorage)
-    new (&op->getOperandStorage()) detail::OperandStorage(op, operands);
+  if (needsOperandStorage) {
+    new (&op->getOperandStorage()) detail::OperandStorage(
+        op, op->getTrailingObjects<OpOperand>(), operands);
+  }
 
   // Initialize the successors.
   auto blockOperands = op->getBlockOperands();
   for (unsigned i = 0; i != numSuccessors; ++i)
     new (&blockOperands[i]) BlockOperand(op, successors[i]);
 
   return op;
 }

 
 // The fallback for the printer is to print in the generic assembly form.
 void OpState::print(Operation *op, OpAsmPrinter &p) { p.printGenericOp(op); }
 // The fallback for the printer is to print in the generic assembly form.
 void OpState::printOpName(Operation *op, OpAsmPrinter &p,
                           StringRef defaultDialect) {
   StringRef name = op->getName().getStringRef();
   if (name.startswith((defaultDialect + ".").str()))
     name = name.drop_front(defaultDialect.size() + 1);
   // TODO: remove this special case.
   else if (name.startswith("std."))
     name = name.drop_front(4);
   p.getStream() << name;
 }
 
 /// Emit an error about fatal conditions with this operation, reporting up to
 /// any diagnostic handlers that may be listening.
 InFlightDiagnostic OpState::emitError(const Twine &message) {

mlir/lib/IR/OperationSupport.cpp

void OperationState::addRegions(
   for (std::unique_ptr<Region> &region : regions)
     addRegion(std::move(region));
 }
 
 //===----------------------------------------------------------------------===//
 // OperandStorage
 //===----------------------------------------------------------------------===//
 
-detail::OperandStorage::OperandStorage(Operation *owner, ValueRange values)
-    : inlineStorage() {
-  auto &inlineStorage = getInlineStorage();
-  inlineStorage.numOperands = inlineStorage.capacity = values.size();
-  auto *operandPtrBegin = getTrailingObjects<OpOperand>();
-  for (unsigned i = 0, e = inlineStorage.numOperands; i < e; ++i)
-    new (&operandPtrBegin[i]) OpOperand(owner, values[i]);
+detail::OperandStorage::OperandStorage(Operation *owner,
+                                       OpOperand *trailingOperands,
+                                       ValueRange values)
+    : isStorageDynamic(false), operandStorage(trailingOperands) {
+  numOperands = capacity = values.size();
+  for (unsigned i = 0; i < numOperands; ++i)
+    new (&operandStorage[i]) OpOperand(owner, values[i]);
 }
 
 detail::OperandStorage::~OperandStorage() {
-  // Destruct the current storage container.
-  if (isDynamicStorage()) {
-    TrailingOperandStorage &storage = getDynamicStorage();
-    storage.~TrailingOperandStorage();
-    // Work around -Wfree-nonheap-object false positive fixed by D102728.
-    auto *mem = &storage;
-    free(mem);
-  } else {
-    getInlineStorage().~TrailingOperandStorage();
-  }
+  for (auto &operand : getOperands())
+    operand.~OpOperand();
+
+  // If the storage is dynamic, deallocate it.
+  if (isStorageDynamic)
+    free(operandStorage);
 }
 
 /// Replace the operands contained in the storage with the ones provided in
 /// 'values'.
 void detail::OperandStorage::setOperands(Operation *owner, ValueRange values) {
   MutableArrayRef<OpOperand> storageOperands = resize(owner, values.size());
   for (unsigned i = 0, e = values.size(); i != e; ++i)
     storageOperands[i].set(values[i]);
void detail::OperandStorage::setOperands(Operation *owner, unsigned start,
 
   // Update the operands inplace.
   for (unsigned i = 0, e = operands.size(); i != e; ++i)
     storageOperands[start + i].set(operands[i]);
 }
 
 /// Erase an operand held by the storage.
 void detail::OperandStorage::eraseOperands(unsigned start, unsigned length) {
-  TrailingOperandStorage &storage = getStorage();
-  MutableArrayRef<OpOperand> operands = storage.getOperands();
+  MutableArrayRef<OpOperand> operands = getOperands();
   assert((start + length) <= operands.size());
-  storage.numOperands -= length;
+  numOperands -= length;
 
   // Shift all operands down if the operand to remove is not at the end.
-  if (start != storage.numOperands) {
+  if (start != numOperands) {
     auto *indexIt = std::next(operands.begin(), start);
     std::rotate(indexIt, std::next(indexIt, length), operands.end());
   }
   for (unsigned i = 0; i != length; ++i)
-    operands[storage.numOperands + i].~OpOperand();
+    operands[numOperands + i].~OpOperand();
 }
 
 void detail::OperandStorage::eraseOperands(
     const llvm::BitVector &eraseIndices) {
-  TrailingOperandStorage &storage = getStorage();
-  MutableArrayRef<OpOperand> operands = storage.getOperands();
+  MutableArrayRef<OpOperand> operands = getOperands();
   assert(eraseIndices.size() == operands.size());
 
   // Check that at least one operand is erased.
   int firstErasedIndice = eraseIndices.find_first();
   if (firstErasedIndice == -1)
     return;
 
   // Shift all of the removed operands to the end, and destroy them.
-  storage.numOperands = firstErasedIndice;
+  numOperands = firstErasedIndice;
   for (unsigned i = firstErasedIndice + 1, e = operands.size(); i < e; ++i)
     if (!eraseIndices.test(i))
-      operands[storage.numOperands++] = std::move(operands[i]);
-  for (OpOperand &operand : operands.drop_front(storage.numOperands))
+      operands[numOperands++] = std::move(operands[i]);
+  for (OpOperand &operand : operands.drop_front(numOperands))
     operand.~OpOperand();
 }
 
 /// Resize the storage to the given size. Returns the array containing the new
 /// operands.
 MutableArrayRef<OpOperand> detail::OperandStorage::resize(Operation *owner,
                                                           unsigned newSize) {
-  TrailingOperandStorage &storage = getStorage();
-
   // If the number of operands is less than or equal to the current amount, we
   // can just update in place.
-  unsigned &numOperands = storage.numOperands;
-  MutableArrayRef<OpOperand> operands = storage.getOperands();
+  MutableArrayRef<OpOperand> origOperands = getOperands();
   if (newSize <= numOperands) {
     // If the number of new size is less than the current, remove any extra
     // operands.
     for (unsigned i = newSize; i != numOperands; ++i)
-      operands[i].~OpOperand();
+      origOperands[i].~OpOperand();
     numOperands = newSize;
-    return operands.take_front(newSize);
+    return origOperands.take_front(newSize);
   }
 
   // If the new size is within the original inline capacity, grow inplace.
-  if (newSize <= storage.capacity) {
-    OpOperand *opBegin = operands.data();
+  if (newSize <= capacity) {
+    OpOperand *opBegin = origOperands.data();
     for (unsigned e = newSize; numOperands != e; ++numOperands)
       new (&opBegin[numOperands]) OpOperand(owner);
     return MutableArrayRef<OpOperand>(opBegin, newSize);
   }
 
   // Otherwise, we need to allocate a new storage.
   unsigned newCapacity =
-      std::max(unsigned(llvm::NextPowerOf2(storage.capacity + 2)), newSize);
-  auto *newStorageMem =
-      malloc(TrailingOperandStorage::totalSizeToAlloc<OpOperand>(newCapacity));
-  auto *newStorage = ::new (newStorageMem) TrailingOperandStorage();
-  newStorage->numOperands = newSize;
-  newStorage->capacity = newCapacity;
+      std::max(unsigned(llvm::NextPowerOf2(capacity + 2)), newSize);
+  OpOperand *newOperandStorage =
+      reinterpret_cast<OpOperand *>(malloc(sizeof(OpOperand) * newCapacity));
 
   // Move the current operands to the new storage.
-  MutableArrayRef<OpOperand> newOperands = newStorage->getOperands();
-  std::uninitialized_copy(std::make_move_iterator(operands.begin()),
-                          std::make_move_iterator(operands.end()),
+  MutableArrayRef<OpOperand> newOperands(newOperandStorage, newSize);
+  std::uninitialized_copy(std::make_move_iterator(origOperands.begin()),
+                          std::make_move_iterator(origOperands.end()),
                           newOperands.begin());
 
   // Destroy the original operands.
-  for (auto &operand : operands)
+  for (auto &operand : origOperands)
     operand.~OpOperand();
 
   // Initialize any new operands.
   for (unsigned e = newSize; numOperands != e; ++numOperands)
     new (&newOperands[numOperands]) OpOperand(owner);
 
-  // If the current storage is also dynamic, free it.
-  if (isDynamicStorage()) {
-    // Work around -Wfree-nonheap-object false positive fixed by D102728.
-    auto *mem = &storage;
-    free(mem);
-  }
+  // If the current storage is dynamic, free it.
+  if (isStorageDynamic)
+    free(operandStorage);
 
   // Update the storage representation to use the new dynamic storage.
-  dynamicStorage.setPointerAndInt(newStorage, true);
+  operandStorage = newOperandStorage;
+  capacity = newCapacity;
+  isStorageDynamic = true;
   return newOperands;
 }
 
 //===----------------------------------------------------------------------===//
 // Operation Value-Iterators
 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//