[mlir][NFC] Split the non-templated bits out of IROperand into a base class

[rriddle]

Summary

This removes the need to define the derived Operand class before the derived
Value class. The major benefit of this refactoring is that we no longer need
the OpaqueValue class, as OpOperand can now be defined after Value. As part of
this refactoring the BlockOperand and OpOperand classes are moved out of
UseDefLists.h and to more suitable locations in BlockSupport and Value. After
this change, UseDefLists.h is almost entirely composed of generic use def utilities.

[lattner]

This is really great River, thank you and nice work!

mlir/include/mlir/IR/BlockSupport.h

37

Nit: "in the BlockOperand list of the Operation".

Unrelated to your patch, but I think we should expunge the term "User" from the MLIR IR definition (e.g. the getUser() method in ValueUseIterator). This is an LLVM term that we outgrew. I think it would be better to just use the word "getOperation()" instead of getOwner() / getUser().

mlir/include/mlir/IR/Value.h

258

nit User -> Operation

[rriddle]

Thanks for the review!

mlir/include/mlir/IR/BlockSupport.h

37

The problematic part of removing the User part to me is what to do with the getUsers API. Would Value::getUsers turn into Value::getOperations? I can see that being confusing, i.e. does that include the parent op? +1 on changing getOwner though.

[lattner]

mlir/include/mlir/IR/BlockSupport.h

37

Good point, I'd keep getUsers() named as it is. The key thing there is that "user" is a description of a class of operation that "use" the value, so the name is appropriate.

mlir/include/mlir/IR/BlockSupport.h

 #include "llvm/ADT/PointerUnion.h"
 #include "llvm/ADT/ilist.h"
 #include "llvm/ADT/ilist_node.h"
 
 namespace mlir {
 class Block;
 
 //===----------------------------------------------------------------------===//
+// BlockOperand
+//===----------------------------------------------------------------------===//
+
+/// A block operand represents an operand that holds a reference to a Block,
+/// e.g. for terminator operations.
+class BlockOperand : public IROperand<BlockOperand, Block *> {
+public:
+  using IROperand<BlockOperand, Block *>::IROperand;
+
+  /// Provide the use list that is attached to the given block.
+  static IRObjectWithUseList<BlockOperand> *getUseList(Block *value);
+
+  /// Return which operand this is in the BlockOperand list of the Operation.
+  unsigned getOperandNumber();
+};
+
+//===----------------------------------------------------------------------===//
 // Predecessors
 //===----------------------------------------------------------------------===//
 
 /// Implement a predecessor iterator for blocks. This works by walking the use
 /// lists of the blocks. The entries on this list are the BlockOperands that
 /// are embedded into terminator operations. From the operand, we can get the
 /// terminator that contains it, and its parent block is the predecessor.
 class PredecessorIterator final

mlir/include/mlir/IR/UseDefLists.h

 #define MLIR_IR_USEDEFLISTS_H
 
 #include "mlir/IR/Location.h"
 #include "llvm/ADT/PointerIntPair.h"
 #include "llvm/ADT/iterator_range.h"
 
 namespace mlir {
 
-class Block;
 class Operation;
-class Value;
 template <typename OperandType> class ValueUseIterator;
 template <typename OperandType> class FilteredValueUseIterator;
 template <typename UseIteratorT, typename OperandType> class ValueUserIterator;
 
 //===----------------------------------------------------------------------===//
-// IRObjectWithUseList
+// IROperand
 //===----------------------------------------------------------------------===//
 
-/// This class represents a single IR object that contains a use list.
-template <typename OperandType> class IRObjectWithUseList {
+namespace detail {
+/// This class is the base for IROperand, and provides all of the non-templated
+/// facilities for operand use management.
+class IROperandBase {
 public:
-  ~IRObjectWithUseList() {
-    assert(use_empty() && "Cannot destroy a value that still has uses!");
-  }
+  /// Return the owner of this operand.
+  Operation *getOwner() const { return owner; }
 
-  /// Drop all uses of this object from their respective owners.
-  void dropAllUses() {
-    while (!use_empty())
-      use_begin()->drop();
-  }
+  /// Return the next operand on the use-list of the value we are referring to.
+  /// This should generally only be used by the internal implementation details
+  /// of the SSA machinery.
+  IROperandBase *getNextOperandUsingThisValue() { return nextUse; }
 
-  /// Replace all uses of 'this' value with the new value, updating anything in
-  /// the IR that uses 'this' to use the other value instead.  When this returns
-  /// there are zero uses of 'this'.
-  void replaceAllUsesWith(typename OperandType::ValueType newValue) {
-    assert((!newValue || this != OperandType::getUseList(newValue)) &&
-           "cannot RAUW a value with itself");
-    while (!use_empty())
-      use_begin()->set(newValue);
+protected:
+  IROperandBase(Operation *owner) : owner(owner) {}
+  IROperandBase(IROperandBase &&other) : owner(other.owner) {
+    *this = std::move(other);
   }
-
-  //===--------------------------------------------------------------------===//
-  // Uses
-  //===--------------------------------------------------------------------===//
-
-  using use_iterator = ValueUseIterator<OperandType>;
-  using use_range = iterator_range<use_iterator>;
-
-  use_iterator use_begin() const { return use_iterator(firstUse); }
-  use_iterator use_end() const { return use_iterator(nullptr); }
-
-  /// Returns a range of all uses, which is useful for iterating over all uses.
-  use_range getUses() const { return {use_begin(), use_end()}; }
-
-  /// Returns true if this value has exactly one use.
-  bool hasOneUse() const {
-    return firstUse && firstUse->getNextOperandUsingThisValue() == nullptr;
+  IROperandBase &operator=(IROperandBase &&other) {
+    removeFromCurrent();
+    other.removeFromCurrent();
+    other.back = nullptr;
+    nextUse = nullptr;
+    back = nullptr;
+    return *this;
   }
+  /// Operands are not copyable or assignable.
+  IROperandBase(const IROperandBase &use) = delete;
+  IROperandBase &operator=(const IROperandBase &use) = delete;
 
-  /// Returns true if this value has no uses.
-  bool use_empty() const { return firstUse == nullptr; }
-
-  //===--------------------------------------------------------------------===//
-  // Users
-  //===--------------------------------------------------------------------===//
+  ~IROperandBase() { removeFromCurrent(); }
 
-  using user_iterator = ValueUserIterator<use_iterator, OperandType>;
-  using user_range = iterator_range<user_iterator>;
+  /// Remove this use of the operand.
+  void drop() {
+    removeFromCurrent();
+    nextUse = nullptr;
+    back = nullptr;
+  }
 
-  user_iterator user_begin() const { return user_iterator(use_begin()); }
-  user_iterator user_end() const { return user_iterator(use_end()); }
+  /// Remove this operand from the current use list.
+  void removeFromCurrent() {
+    if (!back)
+      return;
+    *back = nextUse;
+    if (nextUse)
+      nextUse->back = back;
+  }
 
-  /// Returns a range of all users.
-  user_range getUsers() const { return {user_begin(), user_end()}; }
+  /// Insert this operand into the given use list.
+  template <typename UseListT> void insertInto(UseListT *useList) {
+    back = &useList->firstUse;
+    nextUse = useList->firstUse;
+    if (nextUse)
+      nextUse->back = &nextUse;
+    useList->firstUse = this;
+  }
 
-protected:
-  IRObjectWithUseList() {}
+  /// The next operand in the use-chain.
+  IROperandBase *nextUse = nullptr;
 
-  /// Return the first operand that is using this value, for use by custom
-  /// use/def iterators.
-  OperandType *getFirstUse() const { return firstUse; }
+  /// This points to the previous link in the use-chain.
+  IROperandBase **back = nullptr;
 
 private:
-  template <typename DerivedT, typename IRValueTy> friend class IROperand;
-  OperandType *firstUse = nullptr;
+  /// The operation owner of this operand.
+  Operation *const owner;
 };
+} // namespace detail
 
 //===----------------------------------------------------------------------===//
 // IROperand
 //===----------------------------------------------------------------------===//
 
 /// A reference to a value, suitable for use as an operand of an operation.
-/// IRValueTy is the root type to use for values this tracks. Derived operand
+/// IRValueT is the root type to use for values this tracks. Derived operand
 /// types are expected to provide the following:
-///  * static IRObjectWithUseList *getUseList(IRValueTy value);
+///  * static IRObjectWithUseList *getUseList(IRValueT value);
 ///    - Provide the use list that is attached to the given value.
-template <typename DerivedT, typename IRValueTy> class IROperand {
+template <typename DerivedT, typename IRValueT>
+class IROperand : public detail::IROperandBase {
 public:
-  using ValueType = IRValueTy;
+  IROperand(Operation *owner) : detail::IROperandBase(owner) {}
+  IROperand(Operation *owner, IRValueT value)
+      : detail::IROperandBase(owner), value(value) {
+    insertIntoCurrent();
+  }
 
-  IROperand(Operation *owner) : owner(owner) {}
-  IROperand(Operation *owner, ValueType value) : value(value), owner(owner) {
+  /// We support a move constructor so IROperand's can be in vectors, but this
+  /// shouldn't be used by general clients.
+  IROperand(IROperand &&other) : detail::IROperandBase(std::move(other)) {
+    *this = std::move(other);
+  }
+  IROperand &operator=(IROperand &&other) {
+    detail::IROperandBase::operator=(std::move(other));
+    value = other.value;
+    other.value = nullptr;
+    if (value)
       insertIntoCurrent();
+    return *this;
   }
 
   /// Return the current value being used by this operand.
-  ValueType get() const { return value; }
+  IRValueT get() const { return value; }
 
   /// Set the current value being used by this operand.
-  void set(ValueType newValue) {
+  void set(IRValueT newValue) {
     // It isn't worth optimizing for the case of switching operands on a single
     // value.
     removeFromCurrent();
     value = newValue;
     insertIntoCurrent();
   }
 
   /// Returns true if this operand contains the given value.
-  bool is(ValueType other) const { return value == other; }
-
-  /// Return the owner of this operand.
-  Operation *getOwner() { return owner; }
-  Operation *getOwner() const { return owner; }
+  bool is(IRValueT other) const { return value == other; }
 
   /// \brief Remove this use of the operand.
   void drop() {
-    removeFromCurrent();
+    detail::IROperandBase::drop();
     value = nullptr;
-    nextUse = nullptr;
-    back = nullptr;
-  }
-
-  ~IROperand() { removeFromCurrent(); }
-
-  /// Return the next operand on the use-list of the value we are referring to.
-  /// This should generally only be used by the internal implementation details
-  /// of the SSA machinery.
-  DerivedT *getNextOperandUsingThisValue() { return nextUse; }
-
-  /// We support a move constructor so IROperand's can be in vectors, but this
-  /// shouldn't be used by general clients.
-  IROperand(IROperand &&other) : owner(other.owner) {
-    *this = std::move(other);
-  }
-  IROperand &operator=(IROperand &&other) {
-    removeFromCurrent();
-    other.removeFromCurrent();
-    value = other.value;
-    other.value = nullptr;
-    other.back = nullptr;
-    nextUse = nullptr;
-    back = nullptr;
-    if (value)
-      insertIntoCurrent();
-    return *this;
   }
 
 private:
   /// The value used as this operand. This can be null when in a 'dropAllUses'
   /// state.
-  ValueType value = {};
-
-  /// The next operand in the use-chain.
-  DerivedT *nextUse = nullptr;
+  IRValueT value = {};
 
-  /// This points to the previous link in the use-chain.
-  DerivedT **back = nullptr;
+  /// Insert this operand into the given use list.
+  void insertIntoCurrent() { insertInto(DerivedT::getUseList(value)); }
+};
 
-  /// The operation owner of this operand.
-  Operation *const owner;
+//===----------------------------------------------------------------------===//
+// IRObjectWithUseList
+//===----------------------------------------------------------------------===//
 
-  /// Operands are not copyable or assignable.
-  IROperand(const IROperand &use) = delete;
-  IROperand &operator=(const IROperand &use) = delete;
+/// This class represents a single IR object that contains a use list.
+template <typename OperandType> class IRObjectWithUseList {
+public:
+  ~IRObjectWithUseList() {
+    assert(use_empty() && "Cannot destroy a value that still has uses!");
+  }
 
-  void removeFromCurrent() {
-    if (!back)
-      return;
-    *back = nextUse;
-    if (nextUse)
-      nextUse->back = back;
+  /// Drop all uses of this object from their respective owners.
+  void dropAllUses() {
+    while (!use_empty())
+      use_begin()->drop();
   }
 
-  void insertIntoCurrent() {
-    auto *useList = DerivedT::getUseList(value);
-    back = &useList->firstUse;
-    nextUse = useList->firstUse;
-    if (nextUse)
-      nextUse->back = &nextUse;
-    useList->firstUse = static_cast<DerivedT *>(this);
+  /// Replace all uses of 'this' value with the new value, updating anything in
+  /// the IR that uses 'this' to use the other value instead.  When this returns
+  /// there are zero uses of 'this'.
+  template <typename ValueT>
+  void replaceAllUsesWith(ValueT &&newValue) {
+    assert((!newValue || this != OperandType::getUseList(newValue)) &&
+           "cannot RAUW a value with itself");
+    while (!use_empty())
+      use_begin()->set(newValue);
   }
-};
 
-//===----------------------------------------------------------------------===//
-// BlockOperand
-//===----------------------------------------------------------------------===//
+  //===--------------------------------------------------------------------===//
+  // Uses
+  //===--------------------------------------------------------------------===//
 
-/// Terminator operations can have Block operands to represent successors.
-class BlockOperand : public IROperand<BlockOperand, Block *> {
-public:
-  using IROperand<BlockOperand, Block *>::IROperand;
+  using use_iterator = ValueUseIterator<OperandType>;
+  using use_range = iterator_range<use_iterator>;
 
-  /// Provide the use list that is attached to the given block.
-  static IRObjectWithUseList<BlockOperand> *getUseList(Block *value);
+  use_iterator use_begin() const { return use_iterator(firstUse); }
+  use_iterator use_end() const { return use_iterator(nullptr); }
 
-  /// Return which operand this is in the operand list of the User.
-  unsigned getOperandNumber();
-};
+  /// Returns a range of all uses, which is useful for iterating over all uses.
+  use_range getUses() const { return {use_begin(), use_end()}; }
 
-//===----------------------------------------------------------------------===//
-// OpOperand
-//===----------------------------------------------------------------------===//
+  /// Returns true if this value has exactly one use.
+  bool hasOneUse() const {
+    return firstUse && firstUse->getNextOperandUsingThisValue() == nullptr;
+  }
 
-namespace detail {
-/// This class provides an opaque type erased wrapper around a `Value`.
-class OpaqueValue {
-public:
-  /// Implicit conversion from 'Value'.
-  OpaqueValue(Value value);
-  OpaqueValue(std::nullptr_t = nullptr) : impl(nullptr) {}
-  OpaqueValue(const OpaqueValue &) = default;
-  OpaqueValue &operator=(const OpaqueValue &) = default;
-  bool operator==(const OpaqueValue &other) const { return impl == other.impl; }
-  operator bool() const { return impl; }
+  /// Returns true if this value has no uses.
+  bool use_empty() const { return firstUse == nullptr; }
 
-  /// Implicit conversion back to 'Value'.
-  operator Value() const;
+  //===--------------------------------------------------------------------===//
+  // Users
+  //===--------------------------------------------------------------------===//
 
-private:
-  void *impl;
-};
-} // namespace detail
+  using user_iterator = ValueUserIterator<use_iterator, OperandType>;
+  using user_range = iterator_range<user_iterator>;
 
-/// This class represents an operand of an operation. Instances of this class
-/// contain a reference to a specific `Value`.
-class OpOperand : public IROperand<OpOperand, detail::OpaqueValue> {
-public:
-  /// Provide the use list that is attached to the given value.
-  static IRObjectWithUseList<OpOperand> *getUseList(Value value);
+  user_iterator user_begin() const { return user_iterator(use_begin()); }
+  user_iterator user_end() const { return user_iterator(use_end()); }
 
-  /// Return the current value being used by this operand.
-  Value get() const;
+  /// Returns a range of all users.
+  user_range getUsers() const { return {user_begin(), user_end()}; }
 
-  /// Set the operand to the given value.
-  void set(Value value);
+protected:
+  IRObjectWithUseList() = default;
 
-  /// Return which operand this is in the operand list of the User.
-  unsigned getOperandNumber();
+  /// Return the first operand that is using this value, for use by custom
+  /// use/def iterators.
+  OperandType *getFirstUse() const { return (OperandType *)firstUse; }
 
 private:
-  /// Keep the constructor private and accessible to the OperandStorage class
-  /// only to avoid hard-to-debug typo/programming mistakes.
-  friend class OperandStorage;
-  using IROperand<OpOperand, detail::OpaqueValue>::IROperand;
+  detail::IROperandBase *firstUse = nullptr;
+
+  /// Allow access to `firstUse`.
+  friend detail::IROperandBase;
 };
 
 //===----------------------------------------------------------------------===//
 // ValueUseIterator
 //===----------------------------------------------------------------------===//
 
 /// An iterator class that allows for iterating over the uses of an IR operand
 /// type.
 template <typename OperandType>
 class ValueUseIterator
     : public llvm::iterator_facade_base<ValueUseIterator<OperandType>,
                                         std::forward_iterator_tag,
                                         OperandType> {
 public:
-  ValueUseIterator(OperandType *current = nullptr) : current(current) {}
+  ValueUseIterator(detail::IROperandBase *use = nullptr) : current(use) {}
 
-  /// Returns the user that owns this use.
+  /// Returns the operation that owns this use.
   Operation *getUser() const { return current->getOwner(); }
 
   /// Returns the current operands.
-  OperandType *getOperand() const { return current; }
-  OperandType &operator*() const { return *current; }
+  OperandType *getOperand() const { return (OperandType *)current; }
+  OperandType &operator*() const { return *getOperand(); }
 
   using llvm::iterator_facade_base<ValueUseIterator<OperandType>,
                                    std::forward_iterator_tag,
                                    OperandType>::operator++;
   ValueUseIterator &operator++() {
     assert(current && "incrementing past end()!");
     current = (OperandType *)current->getNextOperandUsingThisValue();
     return *this;
   }
 
   bool operator==(const ValueUseIterator &rhs) const {
     return current == rhs.current;
   }
 
 protected:
-  OperandType *current;
+  detail::IROperandBase *current;
 };
 
 //===----------------------------------------------------------------------===//
 // ValueUserIterator
 //===----------------------------------------------------------------------===//
 
 /// An iterator over the users of an IRObject. This is a wrapper iterator around
 /// a specific use iterator.

mlir/include/mlir/IR/Value.h

 
 #include "mlir/IR/Types.h"
 #include "mlir/IR/UseDefLists.h"
 #include "mlir/Support/LLVM.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
 
 namespace mlir {
 class AsmState;
+class Block;
 class BlockArgument;
 class Operation;
+class OpOperand;
 class OpResult;
 class Region;
 class Value;
 
 //===----------------------------------------------------------------------===//
 // Value
 //===----------------------------------------------------------------------===//
 
public:
 
   /// Return the Block in which this Value is defined.
   Block *getParentBlock();
 
   //===--------------------------------------------------------------------===//
   // UseLists
   //===--------------------------------------------------------------------===//
 
-  /// Provide the use list that is attached to this value.
-  IRObjectWithUseList<OpOperand> *getUseList() const { return impl; }
-
   /// Drop all uses of this object from their respective owners.
-  void dropAllUses() const { return getUseList()->dropAllUses(); }
+  void dropAllUses() const { return impl->dropAllUses(); }
 
   /// Replace all uses of 'this' value with the new value, updating anything in
   /// the IR that uses 'this' to use the other value instead.  When this returns
   /// there are zero uses of 'this'.
-  void replaceAllUsesWith(Value newValue) const;
+  void replaceAllUsesWith(Value newValue) const {
+    impl->replaceAllUsesWith(newValue);
+  }
 
   /// Replace all uses of 'this' value with 'newValue', updating anything in the
   /// IR that uses 'this' to use the other value instead except if the user is
   /// listed in 'exceptions' .
   void
   replaceAllUsesExcept(Value newValue,
                        const SmallPtrSetImpl<Operation *> &exceptions) const;
 
public:
 
   //===--------------------------------------------------------------------===//
   // Uses
 
   /// This class implements an iterator over the uses of a value.
   using use_iterator = ValueUseIterator<OpOperand>;
   using use_range = iterator_range<use_iterator>;
 
-  use_iterator use_begin() const { return getUseList()->use_begin(); }
+  use_iterator use_begin() const { return impl->use_begin(); }
   use_iterator use_end() const { return use_iterator(); }
 
   /// Returns a range of all uses, which is useful for iterating over all uses.
   use_range getUses() const { return {use_begin(), use_end()}; }
 
   /// Returns true if this value has exactly one use.
-  bool hasOneUse() const { return getUseList()->hasOneUse(); }
+  bool hasOneUse() const { return impl->hasOneUse(); }
 
   /// Returns true if this value has no uses.
-  bool use_empty() const { return getUseList()->use_empty(); }
+  bool use_empty() const { return impl->use_empty(); }
 
   //===--------------------------------------------------------------------===//
   // Users
 
   using user_iterator = ValueUserIterator<use_iterator, OpOperand>;
   using user_range = iterator_range<user_iterator>;
 
   user_iterator user_begin() const { return use_begin(); }

 };
 
 inline raw_ostream &operator<<(raw_ostream &os, Value value) {
   value.print(os);
   return os;
 }
 
 //===----------------------------------------------------------------------===//
+// OpOperand
+//===----------------------------------------------------------------------===//
+
+/// This class represents an operand of an operation. Instances of this class
+/// contain a reference to a specific `Value`.
+class OpOperand : public IROperand<OpOperand, Value> {
+public:
+  /// Provide the use list that is attached to the given value.
+  static IRObjectWithUseList<OpOperand> *getUseList(Value value) {
+    return value.getImpl();
+  }
+
+  /// Return which operand this is in the OpOperand list of the Operation.
+  unsigned getOperandNumber();
+
+private:
+  /// Keep the constructor private and accessible to the OperandStorage class
+  /// only to avoid hard-to-debug typo/programming mistakes.
+  friend class OperandStorage;
+  using IROperand<OpOperand, Value>::IROperand;
+};
+
+//===----------------------------------------------------------------------===//
 // BlockArgument
 //===----------------------------------------------------------------------===//
 
 namespace detail {
 /// The internal implementation of a BlockArgument.
 class BlockArgumentImpl : public ValueImpl {
 public:
   static bool classof(const ValueImpl *value) {

mlir/lib/IR/Value.cpp

Block *Value::getParentBlock() {
   return cast<BlockArgument>().getOwner();
 }
 
 //===----------------------------------------------------------------------===//
 // Value::UseLists
 //===----------------------------------------------------------------------===//
 
 /// Replace all uses of 'this' value with the new value, updating anything in
-/// the IR that uses 'this' to use the other value instead.  When this returns
-/// there are zero uses of 'this'.
-void Value::replaceAllUsesWith(Value newValue) const {
-  return getUseList()->replaceAllUsesWith(newValue);
-}
-
-/// Replace all uses of 'this' value with the new value, updating anything in
 /// the IR that uses 'this' to use the other value instead except if the user is
 /// listed in 'exceptions' .
 void Value::replaceAllUsesExcept(
     Value newValue, const SmallPtrSetImpl<Operation *> &exceptions) const {
   for (OpOperand &use : llvm::make_early_inc_range(getUses())) {
     if (exceptions.count(use.getOwner()) == 0)
       use.set(newValue);
   }

 unsigned BlockOperand::getOperandNumber() {
   return this - &getOwner()->getBlockOperands()[0];
 }
 
 //===----------------------------------------------------------------------===//
 // OpOperand
 //===----------------------------------------------------------------------===//
 
-/// Provide the use list that is attached to the given value.
-IRObjectWithUseList<OpOperand> *OpOperand::getUseList(Value value) {
-  return value.getUseList();
-}
-
-/// Return the current value being used by this operand.
-Value OpOperand::get() const {
-  return IROperand<OpOperand, OpaqueValue>::get();
-}
-
-/// Set the operand to the given value.
-void OpOperand::set(Value value) {
-  IROperand<OpOperand, OpaqueValue>::set(value);
-}
-
 /// Return which operand this is in the operand list.
 unsigned OpOperand::getOperandNumber() {
   return this - &getOwner()->getOpOperands()[0];
 }
 
-//===----------------------------------------------------------------------===//
-// OpaqueValue
-//===----------------------------------------------------------------------===//
-
-/// Implicit conversion from 'Value'.
-OpaqueValue::OpaqueValue(Value value) : impl(value.getAsOpaquePointer()) {}
-
-/// Implicit conversion back to 'Value'.
-OpaqueValue::operator Value() const {
-  return Value::getFromOpaquePointer(impl);
-}