[PATCH] D78691: [mlir][EDSC] Retire ValueHandle

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

Thanks, great clean up, I like the simplification very much and happy to see this moving!

Most my comments are nits or ideas for further simplifications. Some bigger cleanup ideas (for separate commits):

- all of `ArrayRef<Value>` can now be replaced with `ValueRange`;
- all of `ArrayRef<Type>` can now be replaced with `TypeRange`;
- there is a couple of functions that became trivial and I wonder if they are worth keeping.

I've also seen some changes from `ArrayRef<ValueHandle *>` to one of `ArrayRef<Value *>`, `SmallVectorImpl<Value> &`, `MutableArrayRef<Value>` and I can't understand the logic behind the individual choices.  My suggestion would be to consistently use one specific type (e.g. `MutableArrayRef<Value>`) eveywhere for consistency.

In D78691#1998456 <https://reviews.llvm.org/D78691#1998456>, @mehdi_amini wrote:

> Thanks for the cleanup! LGTM in principle, I don't know if anyone wants to review this in details...


I did.



================
Comment at: mlir/docs/EDSC.md:18
 
-`mlir::edsc::ValueBuilder` is a generic wrapper for the `mlir::Builder::create`
-method that operates on `ValueHandle` objects and return a single ValueHandle.
-For instructions that return no values or that return multiple values, the
-`mlir::edsc::InstructionBuilder` can be used. Named intrinsics are provided as
+`mlir::ValueBuilder` is a generic wrapper for the `mlir::Builder::create` method
+that operates on `Value` objects and return a single Value. For instructions
----------------
Nit: `mlir::OpBuilder::create`


================
Comment at: mlir/docs/EDSC.md:37
+  Value i(indexType),
               j(indexType),
               lb(f->getArgument(0)),
----------------
Nit: reformat (this is a doc, so not automatic)


================
Comment at: mlir/include/mlir/Dialect/Affine/EDSC/Builders.h:28
 /// *only* way to capture the loop induction variable.
-LoopBuilder makeAffineLoopBuilder(ValueHandle *iv,
-                                  ArrayRef<ValueHandle> lbHandles,
-                                  ArrayRef<ValueHandle> ubHandles,
-                                  int64_t step);
+LoopBuilder makeAffineLoopBuilder(Value *iv, ArrayRef<Value> lbHandles,
+                                  ArrayRef<Value> ubHandles, int64_t step);
----------------
Nit: these are not lbHandles and ubHandles anymore


================
Comment at: mlir/include/mlir/Dialect/LoopOps/EDSC/Builders.h:36
+LoopBuilder makeLoopBuilder(Value *iv, Value lbHandle, Value ubHandle,
+                            Value stepHandle, ArrayRef<Value *> iterArgsHandles,
+                            ValueRange iterArgsInitValues);
----------------
Nit these are not handles anymore


================
Comment at: mlir/include/mlir/Dialect/StandardOps/EDSC/Intrinsics.h:96
+std_cond_br(Value cond, BlockHandle *trueBranch, ArrayRef<Type> trueTypes,
+            ArrayRef<Value *> trueCaptures, ArrayRef<Value> trueOperands,
+            BlockHandle *falseBranch, ArrayRef<Type> falseTypes,
----------------
MutableArrayRef instead of ArrayRef<Value *> ?  Okay for a separate commit, but you seem to be doing this already in loop builders.


================
Comment at: mlir/lib/Dialect/Affine/EDSC/Builders.cpp:103
 template <typename Op>
-static ValueHandle createBinaryHandle(ValueHandle lhs, ValueHandle rhs) {
-  return ValueHandle::create<Op>(lhs.getValue(), rhs.getValue());
+static Value createBinaryHandle(Value lhs, Value rhs) {
+  return ValueBuilder<Op>(lhs, rhs);
----------------
Nit: BinaryHandle isn't a thing anymore. `createBinaryOp` ?  Do we even need now that it's mostly trivial?  Okay for a further cleanup.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:162
     // an n-D loop nest; with or without permutations.
-    nPar > 0 ? O(ivs) = ValueHandle(fillOp.value())
-             : O() = ValueHandle(fillOp.value());
+    nPar > 0 ? O(ivs) = Value(fillOp.value()) : O() = Value(fillOp.value());
   }
----------------
I suppose you can drop the Value(fillOp.value()) magic and just do O(ivs) = fillOp now.

Also, consider using a plain "if" rather than a ternary here for readability.


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:219
+  static Value getConvOpInput(ConvOp convOp, IndexedValueType im,
+                              SmallVectorImpl<Value> &imIdx) {
     // TODO(ntv): add a level of indirection to linalg.generic.
----------------
nit: MutableArrayRef


================
Comment at: mlir/lib/Dialect/Linalg/Transforms/LinalgToLoops.cpp:329
                                        PoolingSumOp op) {
     auto indices = getInputAndOutputIndices(allIvs, op);
+    SmallVector<Value, 8> iIdx = indices.first;
----------------
Nit (for a separate clean up, and based on taste): I'd rather make `getInputAndOutputIndices` return a struct with named fields than a pair of vectors, the types in such fragment are not obvious


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:56
+                                             Value step,
+                                             ArrayRef<Value *> iterArgsHandles,
+                                             ValueRange iterArgsInitValues) {
----------------
Nit: there are not Handles anymore, and how about MutableArrayRef<Value>?


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:118
+      lbHandle, ubHandle, stepHandle, iterArgsInitValues);
+  *iv = Value(forOp.getInductionVar());
+  auto *body = loop::getForInductionVarOwner(*iv).getBody();
----------------
No need for `Value(...)`


================
Comment at: mlir/lib/Dialect/LoopOps/EDSC/Builders.cpp:122
     // Skipping the induction variable.
-    *(iter_args_handles[i]) = ValueHandle(body->getArgument(i + 1));
+    iterArgsHandles[i] = Value(body->getArgument(i + 1));
   }
----------------
Not need for `Value(...)`


================
Comment at: mlir/lib/EDSC/Builders.cpp:164
+                                       ArrayRef<Type> types,
+                                       SmallVectorImpl<Value> &args) {
   assert(!*bh && "BlockHandle already captures a block, use "
----------------
It looks like you could take a MutableArrayRef here instead (the code below assumes vector has enough elements), and ditch the version with `ArrayRef<Value *>` below.

Also, this calls for an `assert(types.size() == args.size())`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78691