[PATCH] D72863: [mlir][Linalg] Add tensor support to Linalg EDSC Builders

[Han-Chung Wang via Phabricator via llvm-commits]

hanchung added inline comments.


================
Comment at: mlir/include/mlir/Dialect/Linalg/EDSC/Builders.h:363
+                                    ArrayRef<Type> resultTensorTypes = {},
+                                    int depth_multiplier = 1,
                                     ArrayRef<int> strides = {},
----------------
s/depth_multiplier/depthMultiplier

Same to others below.


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:50
+ValueHandle
+mlir::edsc::LoopRangeBuilder::operator()(std::function<void(void)> fun) {
   if (fun)
----------------
Please fix it.


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:81
+ValueHandle LoopNestRangeBuilder::LoopNestRangeBuilder::operator()(
+    std::function<void(void)> fun) {
   if (fun)
----------------
Please fix it


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:134-137
+  assert(
+      llvm::all_of(llvm::make_range(outputBuffers.begin(), outputBuffers.end()),
+                   [](Value v) { return v.getType().isa<MemRefType>(); }) &&
+      "output operands must all be buffers.");
----------------
Do we also check if all types in resultTensorTypes are RankedTensorType?


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:144
   getMaxDimIndex(inputs, maxPos);
-  getMaxDimIndex(outputs, maxPos);
+  getMaxDimIndex(outputBuffers, maxPos);
   // maxPos is 0 indexed, need to turn this into a count (i.e. +1)
----------------
Do we need to update maxPos with resultTensorTypes?


================
Comment at: mlir/lib/Dialect/Linalg/EDSC/Builders.cpp:229
+  }
+  return makeGenericLinalgOp(iterTypes, {I, O}, {}, resultTensorTypes, fun);
 }
----------------
If this is a unary operation, why do we pass two input to the function?
I was expecting something like `makeGenericLinalgOp(iterTypes, {I}, {}, resultTensorTypes, fun);`
In addition, is there a test for this path?


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:910
+// CHECK-SAME: iterator_types = ["parallel", "parallel", "reduction"]}
+///      CHECK:   ^bb0(%[[a0:.*]]: f32, %[[a1:.*]]: f32, %[[a2:.*]]: f32):
+//       CHECK:     %[[a3:.*]] = mulf %[[a0]], %[[a1]] : f32
----------------
Remove one "/" in the beginning?


================
Comment at: mlir/test/EDSC/builder-api-test.cpp:959
   ScopedContext scope(builder, f.getLoc());
-  linalg_conv_nhwc(makeValueHandles(llvm::to_vector<3>(f.getArguments())),
+  linalg_conv_nhwc(makeValueHandles(llvm::to_vector<3>(f.getArguments())), {},
                    /*strides=*/{3, 4}, /*dilations=*/{5, 6});
----------------
nit: add comment for function argument, /*resultTensorTypes=*/{}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72863