[mlir] Reintroduce API for creating operations with a DictionaryAttr

[Mogball]

Summary

This patch reintroduces an API to create operations with a pre-existing
DictionaryAttr. This API does not populate the attributes with any
default attributes the operation may have, like the API that takes a
NamedAttrList does. NamedAttrList is effective at not re-hashing the
attributes if no default attributes were added, but this new API speeds
up clone-heavy workloads slightly (~5%).

[mehdi_amini]

mlir/include/mlir/IR/Operation.h

86

I'm a bit concerned about the discrepancy of behavior between the two APIs with respect to default attributes. We have now two method with the same name, differing only by taking a NamedAttrList vs a DictionaryAttr, but with a subtle difference that isn't well conveyed in the API.

I don't believe it is common in the code base that we use NamedAttrList vs a DictionaryAttr to signal semantics difference, do we?

[Mogball]

mlir/include/mlir/IR/Operation.h

86

I don't believe it is common in the code base that we use NamedAttrList vs a DictionaryAttr to signal semantics difference, do we?

It isn't. Would it be better to rename the methods? createWithDefaultAttrs for the former?

mlir/include/mlir/IR/Operation.h

 ///
 /// Finally an Operation also contain an optional `DictionaryAttr`, a Location,
 /// and a pointer to its parent Block (if any).
 class alignas(8) Operation final
     : public llvm::ilist_node_with_parent<Operation, Block>,
       private llvm::TrailingObjects<Operation, detail::OperandStorage,
                                     BlockOperand, Region, OpOperand> {
 public:
-  /// Create a new Operation with the specific fields.
+  /// Create a new Operation with the specific fields. This constructor
+  /// populates the provided attribute list with default attributes if
+  /// necessary.
   static Operation *create(Location location, OperationName name,
                            TypeRange resultTypes, ValueRange operands,
                            NamedAttrList &&attributes, BlockRange successors,
                            unsigned numRegions);
 
+  /// Create a new Operation with the specific fields. This constructor uses an
+  /// existing attribute dictionary to avoid uniquing a list of attributes.
+  static Operation *create(Location location, OperationName name,
+                           TypeRange resultTypes, ValueRange operands,
+                           DictionaryAttr attributes, BlockRange successors,
+                           unsigned numRegions);
+
   /// Create a new Operation from the fields stored in `state`.
   static Operation *create(const OperationState &state);
 
   /// Create a new Operation with the specific fields.
   static Operation *create(Location location, OperationName name,
                            TypeRange resultTypes, ValueRange operands,
                            NamedAttrList &&attributes,
                            BlockRange successors = {},
void setDialectAttrs(DialectAttrT &&dialectAttrs) {
     for (auto attr : getAttrs())
       if (!attr.getName().strref().contains('.'))
         attrs.push_back(attr);
     setAttrs(attrs.getDictionary(getContext()));
   }
 
   /// Sets default attributes on unset attributes.
   void populateDefaultAttrs() {
-      NamedAttrList attrs(getAttrDictionary());
-      name.populateDefaultAttrs(attrs);
-      setAttrs(attrs.getDictionary(getContext()));
   }
 
   //===--------------------------------------------------------------------===//
   // Blocks
   //===--------------------------------------------------------------------===//
 
   /// Returns the number of regions held by this operation.
   unsigned getNumRegions() { return numRegions; }

mlir/lib/IR/Operation.cpp

Operation *Operation::create(Location location, OperationName name,
   Operation *op = create(location, name, resultTypes, operands,
                          std::move(attributes), successors, numRegions);
   for (unsigned i = 0; i < numRegions; ++i)
     if (regions[i])
       op->getRegion(i).takeBody(*regions[i]);
   return op;
 }
 
+/// Create a new Operation with the specific fields.
+Operation *Operation::create(Location location, OperationName name,
+                             TypeRange resultTypes, ValueRange operands,
+                             NamedAttrList &&attributes, BlockRange successors,
+                             unsigned numRegions) {
+  // Populate default attributes.
+  name.populateDefaultAttrs(attributes);
+
+  return create(location, name, resultTypes, operands,
+                attributes.getDictionary(location.getContext()), successors,
+                numRegions);
+}
+
 /// Overload of create that takes an existing DictionaryAttr to avoid
 /// unnecessarily uniquing a list of attributes.
 Operation *Operation::create(Location location, OperationName name,
                              TypeRange resultTypes, ValueRange operands,
-                             NamedAttrList &&attributes, BlockRange successors,
+                             DictionaryAttr attributes, BlockRange successors,
                              unsigned numRegions) {
   assert(llvm::all_of(resultTypes, [](Type t) { return t; }) &&
          "unexpected null result type");
 
   // We only need to allocate additional memory for a subset of results.
   unsigned numTrailingResults = OpResult::getNumTrailing(resultTypes.size());
   unsigned numInlineResults = OpResult::getNumInline(resultTypes.size());
   unsigned numSuccessors = successors.size();
size_t byteSize =
       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;
 
-  // Populate default attributes.
-  name.populateDefaultAttrs(attributes);
-
   // Create the new Operation.
-  Operation *op = ::new (rawMem) Operation(
-      location, name, numResults, numSuccessors, numRegions,
-      attributes.getDictionary(location.getContext()), needsOperandStorage);
+  Operation *op =
+      ::new (rawMem) Operation(location, name, numResults, numSuccessors,
+                               numRegions, attributes, needsOperandStorage);
 
   assert((numSuccessors == 0 || op->mightHaveTrait<OpTrait::IsTerminator>()) &&
          "unexpected successors in a non-terminator operation");
 
   // Initialize the results.
   auto resultTypeIt = resultTypes.begin();
   for (unsigned i = 0; i < numInlineResults; ++i, ++resultTypeIt)
     new (op->getInlineOpResult(i)) detail::InlineOpResult(*resultTypeIt, i);