[PATCH] D78892: [mlir] Add a new MutableOperandRange class for adding/remove operands

[Jacques Pienaar via Phabricator via llvm-commits]

jpienaar added a comment.

First scan looks good :)



================
Comment at: mlir/include/mlir/IR/Operation.h:214
+  /// Insert the given operands into the operand list at the given 'index'.
+  void insertOperands(unsigned index, ValueRange operands);
+
----------------
How does this affects ops with AttrSizedOperandSegments? E.g., if someone were to call this function manually on an op that implements AttrSizedOperandSegments and inserts an operand in the middle of a segment, what happens?


================
Comment at: mlir/include/mlir/IR/OperationSupport.h:699
+
+  Operation *owner;
+  unsigned start, length;
----------------
Comments for these?


================
Comment at: mlir/lib/IR/OperationSupport.cpp:316
+
+/// Allow implicit conversion to a proper OperandRange.
+MutableOperandRange::operator OperandRange() const {
----------------
Nit: s/proper// (proper always makes me think of something invalid, we are converting to an OperandRange and we wouldn't be creating an improper one so seems already captured)


================
Comment at: mlir/lib/IR/OperationSupport.cpp:327
+
+  auto attr =
+      owner->getAttr("operand_segment_sizes").cast<DenseIntElementsAttr>();
----------------
Could we use AttrSizedOperandSegments's getOperandSegmentSizeAttr() instead? [I'm assuming the templating is what makes it ugly I know I did a "creative solution" equivalent to AttrSizedOperandSegments<void>::getOperandSegmentSizeAttr() in another such case]. But we already have this string in 2 places hardcoded. Could also be done in follow up

Could we alternatively pass in an optional NamedAttribute along with attrSizedOperandSgement in the constructor and at the call site query the attribute and reuse the name here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78892/new/

https://reviews.llvm.org/D78892