IR: Move MDNode operands from the back to the front

Having the operands at the back prevents subclasses from safely adding
fields.  Move them to the front.

Instead of replicating the custom `malloc()`, `free()` and `DestroyFlag`
logic that was there before, overload `new` and `delete`.

I added calls to a new `GenericMDNode::dropAllReferences()` in
`LLVMContextImpl::~LLVMContextImpl()`.  There's a maze of callbacks
happening during teardown, and this resolves them before we enter
the destructors.

Part of PR21532.

llvm-svn: 222211

diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
index b35165a..6da5b21 100644 (file)
--- a/llvm/include/llvm/IR/Metadata.h
+++ b/llvm/include/llvm/IR/Metadata.h
@@ -22,6 +22,7 @@
 #include "llvm/ADT/ilist_node.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/IR/Value.h"
+#include "llvm/Support/ErrorHandling.h"
 
 namespace llvm {
 class LLVMContext;
@@ -145,8 +146,24 @@ class MDNode : public Metadata {
   void operator=(const MDNode &) LLVM_DELETED_FUNCTION;
   friend class MDNodeOperand;
   friend class LLVMContextImpl;
+  void *operator new(size_t) LLVM_DELETED_FUNCTION;
 
 protected:
+  void *operator new(size_t Size, unsigned NumOps);
+
+  /// \brief Required by std, but never called.
+  void operator delete(void *Mem);
+
+  /// \brief Required by std, but never called.
+  void operator delete(void *, unsigned) {
+    llvm_unreachable("Constructor throws?");
+  }
+
+  /// \brief Required by std, but never called.
+  void operator delete(void *, unsigned, bool) {
+    llvm_unreachable("Constructor throws?");
+  }
+
   // TODO: Sink this into GenericMDNode.  Can't do this until operands are
   // allocated at the front (currently they're at the back).
   unsigned Hash;
@@ -160,11 +177,7 @@ protected:
 
     /// NotUniquedBit - This is set on MDNodes that are not uniqued because they
     /// have a null operand.
-    NotUniquedBit    = 1 << 1,
-
-    /// DestroyFlag - This bit is set by destroy() so the destructor can assert
-    /// that the node isn't being destroyed with a plain 'delete'.
-    DestroyFlag      = 1 << 2
+    NotUniquedBit    = 1 << 1
   };
 
   /// \brief FunctionLocal enums.
@@ -176,10 +189,10 @@ protected:
 
   /// \brief Replace each instance of the given operand with a new value.
   void replaceOperand(MDNodeOperand *Op, Value *NewVal);
-  ~MDNode();
 
   MDNode(LLVMContext &C, unsigned ID, ArrayRef<Value *> Vals,
          bool isFunctionLocal);
+  ~MDNode() {}
 
   static MDNode *getMDNode(LLVMContext &C, ArrayRef<Value*> Vals,
                            FunctionLocalness FL, bool Insert = true);
@@ -270,11 +283,14 @@ protected:
 /// TODO: Drop support for RAUW.
 class GenericMDNode : public MDNode {
   friend class MDNode;
+  friend class LLVMContextImpl;
 
   GenericMDNode(LLVMContext &C, ArrayRef<Value *> Vals, bool isFunctionLocal)
       : MDNode(C, GenericMDNodeVal, Vals, isFunctionLocal) {}
   ~GenericMDNode();
 
+  void dropAllReferences();
+
 public:
   /// \brief Get the hash, if any.
   unsigned getHash() const { return Hash; }
@@ -282,12 +298,6 @@ public:
   static bool classof(const Value *V) {
     return V->getValueID() == GenericMDNodeVal;
   }
-
-private:
-  /// \brief Delete this node.  Only when there are no uses.
-  void destroy();
-  friend class MDNode;
-  friend class LLVMContextImpl;
 };
 
 /// \brief Forward declaration of metadata.
@@ -306,11 +316,6 @@ public:
   static bool classof(const Value *V) {
     return V->getValueID() == MDNodeFwdDeclVal;
   }
-
-private:
-  /// \brief Delete this node.  Only when there are no uses.
-  void destroy();
-  friend class MDNode;
 };
 
 //===----------------------------------------------------------------------===//

diff --git a/llvm/lib/IR/LLVMContextImpl.cpp b/llvm/lib/IR/LLVMContextImpl.cpp
index d4239e5..3fd0bb3 100644 (file)
--- a/llvm/lib/IR/LLVMContextImpl.cpp
+++ b/llvm/lib/IR/LLVMContextImpl.cpp
@@ -126,8 +126,10 @@ LLVMContextImpl::~LLVMContextImpl() {
   MDNodes.reserve(MDNodeSet.size() + NonUniquedMDNodes.size());
   MDNodes.append(MDNodeSet.begin(), MDNodeSet.end());
   MDNodes.append(NonUniquedMDNodes.begin(), NonUniquedMDNodes.end());
-  for (auto &I : MDNodes)
-    I->destroy();
+  for (GenericMDNode *I : MDNodes)
+    I->dropAllReferences();
+  for (GenericMDNode *I : MDNodes)
+    delete I;
   assert(MDNodeSet.empty() && NonUniquedMDNodes.empty() &&
          "Destroying all MDNodes didn't empty the Context's sets.");
 

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 3c6c846..b4da2e0 100644 (file)
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -66,25 +66,25 @@ class MDNodeOperand : public CallbackVH {
     MDNodeOperand *Cur = this;
 
     while (Cur->getValPtrInt() != 1)
-      --Cur;
+      ++Cur;
 
     assert(Cur->getValPtrInt() == 1 &&
-           "Couldn't find the beginning of the operand list!");
-    return reinterpret_cast<MDNode*>(Cur) - 1;
+           "Couldn't find the end of the operand list!");
+    return reinterpret_cast<MDNode *>(Cur + 1);
   }
 
 public:
-  MDNodeOperand(Value *V) : CallbackVH(V) {}
+  MDNodeOperand() {}
   virtual ~MDNodeOperand();
 
   void set(Value *V) {
-    unsigned IsFirst = this->getValPtrInt();
+    unsigned IsLast = this->getValPtrInt();
     this->setValPtr(V);
-    this->setAsFirstOperand(IsFirst);
+    this->setAsLastOperand(IsLast);
   }
 
   /// \brief Accessor method to mark the operand as the first in the list.
-  void setAsFirstOperand(unsigned V) { this->setValPtrInt(V); }
+  void setAsLastOperand(unsigned I) { this->setValPtrInt(I); }
 
   void deleted() override;
   void allUsesReplacedWith(Value *NV) override;
@@ -108,14 +108,9 @@ void MDNodeOperand::allUsesReplacedWith(Value *NV) {
 
 /// \brief Get the MDNodeOperand's coallocated on the end of the MDNode.
 static MDNodeOperand *getOperandPtr(MDNode *N, unsigned Op) {
-  static_assert(sizeof(GenericMDNode) == sizeof(MDNode),
-                "Expected subclasses to have no size overhead");
-  static_assert(sizeof(MDNodeFwdDecl) == sizeof(MDNode),
-                "Expected subclasses to have no size overhead");
-
   // Use <= instead of < to permit a one-past-the-end address.
   assert(Op <= N->getNumOperands() && "Invalid operand number");
-  return reinterpret_cast<MDNodeOperand*>(N + 1) + Op;
+  return reinterpret_cast<MDNodeOperand *>(N) - N->getNumOperands() + Op;
 }
 
 void MDNode::replaceOperandWith(unsigned i, Value *Val) {
@@ -123,6 +118,26 @@ void MDNode::replaceOperandWith(unsigned i, Value *Val) {
   replaceOperand(Op, Val);
 }
 
+void *MDNode::operator new(size_t Size, unsigned NumOps) {
+  void *Ptr = ::operator new(Size + NumOps * sizeof(MDNodeOperand));
+  MDNodeOperand *Op = static_cast<MDNodeOperand *>(Ptr);
+  if (NumOps) {
+    MDNodeOperand *Last = Op + NumOps;
+    for (; Op != Last; ++Op)
+      new (Op) MDNodeOperand();
+    (Op - 1)->setAsLastOperand(1);
+  }
+  return Op;
+}
+
+void MDNode::operator delete(void *Mem) {
+  MDNode *N = static_cast<MDNode *>(Mem);
+  MDNodeOperand *Op = static_cast<MDNodeOperand *>(Mem);
+  for (unsigned I = 0, E = N->NumOperands; I != E; ++I)
+    (--Op)->~MDNodeOperand();
+  ::operator delete(Op);
+}
+
 MDNode::MDNode(LLVMContext &C, unsigned ID, ArrayRef<Value *> Vals,
                bool isFunctionLocal)
     : Metadata(C, ID), Hash(0) {
@@ -131,16 +146,11 @@ MDNode::MDNode(LLVMContext &C, unsigned ID, ArrayRef<Value *> Vals,
   if (isFunctionLocal)
     setValueSubclassData(getSubclassDataFromValue() | FunctionLocalBit);
 
-  // Initialize the operand list, which is co-allocated on the end of the node.
+  // Initialize the operand list.
   unsigned i = 0;
-  for (MDNodeOperand *Op = getOperandPtr(this, 0), *E = Op+NumOperands;
-       Op != E; ++Op, ++i) {
-    new (Op) MDNodeOperand(Vals[i]);
-
-    // Mark the first MDNodeOperand as being the first in the list of operands.
-    if (i == 0)
-      Op->setAsFirstOperand(1);
-  }
+  for (MDNodeOperand *Op = getOperandPtr(this, 0), *E = Op + NumOperands;
+       Op != E; ++Op, ++i)
+    Op->set(Vals[i]);
 }
 
 GenericMDNode::~GenericMDNode() {
@@ -152,14 +162,10 @@ GenericMDNode::~GenericMDNode() {
   }
 }
 
-MDNode::~MDNode() {
-  assert((getSubclassDataFromValue() & DestroyFlag) != 0 &&
-         "Not being destroyed through destroy()?");
-
-  // Destroy the operands.
-  for (MDNodeOperand *Op = getOperandPtr(this, 0), *E = Op+NumOperands;
+void GenericMDNode::dropAllReferences() {
+  for (MDNodeOperand *Op = getOperandPtr(this, 0), *E = Op + NumOperands;
        Op != E; ++Op)
-    Op->~MDNodeOperand();
+    Op->set(nullptr);
 }
 
 static const Function *getFunctionForValue(Value *V) {
@@ -216,21 +222,6 @@ const Function *MDNode::getFunction() const {
 #endif
 }
 
-// destroy - Delete this node.  Only when there are no uses.
-void GenericMDNode::destroy() {
-  setValueSubclassData(getSubclassDataFromValue() | DestroyFlag);
-  // Placement delete, then free the memory.
-  this->~GenericMDNode();
-  free(this);
-}
-
-void MDNodeFwdDecl::destroy() {
-  setValueSubclassData(getSubclassDataFromValue() | DestroyFlag);
-  // Placement delete, then free the memory.
-  this->~MDNodeFwdDecl();
-  free(this);
-}
-
 /// \brief Check if the Value  would require a function-local MDNode.
 static bool isFunctionLocalValue(Value *V) {
   return isa<Instruction>(V) || isa<Argument>(V) || isa<BasicBlock>(V) ||
@@ -268,9 +259,8 @@ MDNode *MDNode::getMDNode(LLVMContext &Context, ArrayRef<Value*> Vals,
   }
 
   // Coallocate space for the node and Operands together, then placement new.
-  void *Ptr =
-      malloc(sizeof(GenericMDNode) + Vals.size() * sizeof(MDNodeOperand));
-  GenericMDNode *N = new (Ptr) GenericMDNode(Context, Vals, isFunctionLocal);
+  GenericMDNode *N =
+      new (Vals.size()) GenericMDNode(Context, Vals, isFunctionLocal);
 
   N->Hash = Key.Hash;
   Store.insert(N);
@@ -292,11 +282,8 @@ MDNode *MDNode::getIfExists(LLVMContext &Context, ArrayRef<Value*> Vals) {
 }
 
 MDNode *MDNode::getTemporary(LLVMContext &Context, ArrayRef<Value*> Vals) {
-  MDNode *N = (MDNode *)malloc(sizeof(MDNodeFwdDecl) +
-                               Vals.size() * sizeof(MDNodeOperand));
-  N = new (N) MDNodeFwdDecl(Context, Vals, FL_No);
-  N->setValueSubclassData(N->getSubclassDataFromValue() |
-                          NotUniquedBit);
+  MDNode *N = new (Vals.size()) MDNodeFwdDecl(Context, Vals, FL_No);
+  N->setValueSubclassData(N->getSubclassDataFromValue() | NotUniquedBit);
   LeakDetector::addGarbageObject(N);
   return N;
 }
@@ -306,10 +293,8 @@ void MDNode::deleteTemporary(MDNode *N) {
   assert(isa<MDNodeFwdDecl>(N) && "Expected forward declaration");
   assert((N->getSubclassDataFromValue() & NotUniquedBit) &&
          "Temporary MDNode does not have NotUniquedBit set!");
-  assert((N->getSubclassDataFromValue() & DestroyFlag) == 0 &&
-         "Temporary MDNode has DestroyFlag set!");
   LeakDetector::removeGarbageObject(N);
-  cast<MDNodeFwdDecl>(N)->destroy();
+  delete cast<MDNodeFwdDecl>(N);
 }
 
 /// \brief Return specified operand.
@@ -384,7 +369,7 @@ void MDNode::replaceOperand(MDNodeOperand *Op, Value *To) {
   auto I = Store.find_as(Key);
   if (I != Store.end()) {
     N->replaceAllUsesWith(*I);
-    N->destroy();
+    delete N;
     return;
   }