[mlir][linalg] Introduce new linalg.conv op
#117688
Conversation
This patch lays the groundwork for the new `linalg.conv` op which is designed to replace the multitude of `linalg.conv_...` as well as `linalg.depthwise_conv_...` ops. A test pass is implemented which can convert the old conv ops to the new op. The `linalg-generalize-named-ops` can then be used to convert both the old and the new ops to a `linalg.generic` op for comparison.
ubfx
requested review from
dcaballe,
nicolasvasilache and
rengolin
as code owners
|
@llvm/pr-subscribers-mlir-linalg @llvm/pr-subscribers-mlir-core Author: Felix Schneider (ubfx) ChangesThis patch lays the groundwork for the new A test pass is implemented which can convert the old conv ops to the new op. The Patch is 71.83 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117688.diff 10 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgBase.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgBase.td
index 73f984dc072d31..b659241b5ed5b7 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgBase.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgBase.td
@@ -81,4 +81,43 @@ def IteratorTypeEnum : EnumAttr<Linalg_Dialect, IteratorType, "iterator_type"> {
def IteratorTypeArrayAttr : TypedArrayAttrBase<IteratorTypeEnum,
"Iterator type should be an enum.">;
+
+def ConvolutionDimArray : ArrayRefParameter<"ConvDimEnum"> {
+ let printer = [{
+ $_printer << '{';
+ llvm::interleaveComma($_self, $_printer, [&](ConvDimEnum en) {
+ $_printer.printStrippedAttrOrType(en);
+ });
+ $_printer << '}';
+ }];
+
+ let parser = [{
+ [&]() -> FailureOr<SmallVector<ConvDimEnum>> {
+ using Result = SmallVector<ConvDimEnum>;
+ if ($_parser.parseLBrace())
+ return failure();
+ FailureOr<Result> result = FieldParser<Result>::parse($_parser);
+ if (failed(result))
+ return failure();
+ if ($_parser.parseRBrace())
+ return failure();
+ return result;
+ }()
+ }];
+}
+
+/// Attribute that represents an ordered set of tensor dimensions involved in
+/// convolution.
+def ConvDimsAttr : AttrDef<Linalg_Dialect, "ConvDims", [], "::mlir::Attribute"> {
+ let mnemonic = "conv_dims";
+
+ let parameters = (ins
+ ConvolutionDimArray:$dims
+ );
+
+ let assemblyFormat = "$dims";
+
+ let returnType = "mlir::linalg::ConvDims";
+ let convertFromStorage = "mlir::linalg::ConvDims($_self.getDims())";
+}
#endif // LINALG_BASE
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgEnums.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgEnums.td
index e615876a95d057..ef9e00822fbe3b 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgEnums.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgEnums.td
@@ -63,4 +63,30 @@ def TypeFn : I32EnumAttr<"TypeFn", "", [
let cppNamespace = "::mlir::linalg";
}
+
+class ConvDimEnumAttrCase<string sym, int val, string str = sym>
+ : IntEnumAttrCaseBase<I8, sym, str, val>;
+
+def ConvDimEnumAttr :
+ IntEnumAttr<I8, "ConvDimEnum", "summary", [
+ /// Batch is a dimension of input and output, indexed from a parallel loop.
+ ConvDimEnumAttrCase<"BATCH", 0, "N">,
+ /// Input channel is a dimension in all tensors, indexed from a reduction loop.
+ /// Depthwise convolutions perform no reduction across channels and therefore
+ /// do not use this.
+ ConvDimEnumAttrCase<"INPUT_CHANNEL", 1, "C">,
+ /// Output channel is a dimension in filter and output, index from a parallel loop.
+ ConvDimEnumAttrCase<"OUTPUT_CHANNEL", 2, "F">,
+ /// Group is a dimension in all tensors and indexed from a parallel loop.
+ ConvDimEnumAttrCase<"GROUP", 3, "G">,
+ /// Spatial dimensions occur in all tensors. Output is indexed from a parallel
+ /// loop, filter from a reduction loop and input from both.
+ ConvDimEnumAttrCase<"SPATIAL_0", 4, "0">,
+ ConvDimEnumAttrCase<"SPATIAL_1", 5, "1">,
+ ConvDimEnumAttrCase<"SPATIAL_2", 6, "2">,
+ ]> {
+ let underlyingType = "uint8_t";
+ let cppNamespace = "::mlir::linalg";
+}
+
#endif // LINALG_ENUMS
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
index 6f1c243cc4396d..752fcd8affaa27 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h
@@ -117,6 +117,33 @@ FailureOr<ConvolutionDimensions> inferConvolutionDims(LinalgOp linalgOp);
bool isaConvolutionOpInterface(LinalgOp linalgOp,
bool allowEmptyConvolvedDims = false);
+enum class ConvDimEnum : uint8_t;
+class ConvDims {
+ ArrayRef<ConvDimEnum> storage;
+
+public:
+ ConvDims() = default;
+ ConvDims(ArrayRef<ConvDimEnum> dims) : storage(dims) {}
+ ConvDims(SmallVectorImpl<ConvDimEnum> &dims) : storage(dims) {}
+
+ bool contains(ConvDimEnum dim) const {
+ return llvm::is_contained(storage, dim);
+ }
+
+ int64_t getPos(ConvDimEnum dim) const {
+ auto it = llvm::find(storage, dim);
+ assert(it != storage.end() && "expected dimension to be present");
+
+ return std::distance(storage.begin(), it);
+ }
+
+ int64_t size() const { return storage.size(); }
+ operator ArrayRef<ConvDimEnum>() const { return storage; }
+
+ auto begin() const { return storage.begin(); }
+ auto end() const { return storage.end(); }
+};
+
/// Checks whether `linalgOp` is semantically equivalent to a `linalg.copyOp`.
bool isaCopyOpInterface(LinalgOp linalgOp);
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
index 37eec6e07963b1..09b2dfd75cf67e 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
@@ -683,6 +683,122 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
}];
}
+//===----------------------------------------------------------------------===//
+// Op definition for ConvOp
+//===----------------------------------------------------------------------===//
+
+def ConvOp : LinalgStructuredBase_Op<"conv", [AttrSizedOperandSegments]> {
+
+ let summary = [{
+ Configurable convolution operation with configurable tensor layouts.
+ }];
+ let description = [{
+ Numeric casting is performed on the operands to the inner multiply,
+ promoting them to the same data type as the accumulator/output.
+
+ The subtype of convolution is defined by the tensor layouts of `input`,
+ `filter`, and `output`. For example, a standard batched 2D convolution:
+
+ ```
+ %0 = linalg.conv {
+ input_dims = #linalg<conv_dims {N, C, "1", "0"}>,
+ filter_dims = #linalg<conv_dims {F, C, "1", "0"}>,
+ output_dims = #linalg<conv_dims {N, F, "1", "0"}>
+ }
+ ins(%input, %filter : tensor<8x4x16x16xf32>, tensor<16x4x3x3xf32>)
+ outs(%output : tensor<8x16x14x14xf32>) -> tensor<8x16x14x14xf32>
+ ```
+
+ This op could be turned into a depthwise convolution as follows:
+ ```
+ %0 = linalg.conv {
+ input_dims = #linalg<conv_dims {N, G, "1", "0"}>,
+ filter_dims = #linalg<conv_dims {G, "1", "0"}>,
+ output_dims = #linalg<conv_dims {N, G, "1", "0"}>
+ }
+ ins(%input, %filter : tensor<8x4x16x16xf32>, tensor<4x3x3xf32>)
+ outs(%output : tensor<8x4x14x14xf32>) -> tensor<8x4x14x14xf32>
+ ```
+
+ For the detailed semantics of the available tensor dimensions, refer to
+ `mlir::linalg::ConvDimsEnum`.
+
+ Strides and dilations can be supplied as optional attributes, where
+ `strides[0]` is the stride for the `SPATIAL_0` dimension, etc.
+ }];
+
+ let arguments = (ins
+ Variadic<AnyType>:$inputs, Variadic<AnyShaped>:$outputs,
+ ConvDimsAttr:$input_dims, ConvDimsAttr:$filter_dims, ConvDimsAttr:$output_dims,
+ OptionalAttr<I64ElementsAttr>:$strides, OptionalAttr<I64ElementsAttr>:$dilations
+ );
+ let results = (outs Variadic<AnyRankedTensor>:$result_tensors);
+ let regions = (region AnyRegion:$region);
+
+ let skipDefaultBuilders = 1;
+ let builders = [
+ OpBuilder<
+ (ins "TypeRange":$resTys, "Value":$input, "Value":$filter, "Value":$output, "ConvDims":$input_dims,
+ "ConvDims":$filter_dims, "ConvDims":$output_dims, "ArrayRef<int64_t>":$strides,
+ "ArrayRef<int64_t>":$dilations, CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
+ [{
+ buildConvOp($_builder, $_state, resTys, input, filter, output,
+ input_dims, filter_dims, output_dims, strides, dilations,
+ attributes, ConvOp::getRegionBuilder());
+ }]>,
+ OpBuilder<
+ (ins "ValueRange":$inputs, "ValueRange":$outputs, "ConvDimsAttr":$input_dims,
+ "ConvDimsAttr":$filter_dims, "ConvDimsAttr":$output_dims,
+ CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
+ [{
+ buildConvOp($_builder, $_state, std::nullopt, inputs, outputs,
+ input_dims, filter_dims, output_dims, nullptr, nullptr,
+ attributes, ConvOp::getRegionBuilder());
+ }]>,
+ OpBuilder<
+ (ins "TypeRange":$resultTensorTypes, "ValueRange":$inputs,
+ "ValueRange":$outputs, "ConvDimsAttr":$input_dims,
+ "ConvDimsAttr":$filter_dims, "ConvDimsAttr":$output_dims,
+ CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
+ [{
+ buildConvOp($_builder, $_state, resultTensorTypes,
+ inputs, outputs, input_dims, filter_dims, output_dims, nullptr, nullptr,
+ attributes, ConvOp::getRegionBuilder());
+ }]>
+ ];
+ let hasCustomAssemblyFormat = 1;
+ let hasFolder = 1;
+ let hasVerifier = 1;
+
+ let extraClassDeclaration = structuredOpsBaseDecls # [{
+ SmallVector<utils::IteratorType> getIteratorTypesArray();
+ ArrayAttr getIndexingMaps();
+
+ /// Implements the block region builder.
+ static void regionBuilder(ImplicitLocOpBuilder &b,
+ Block &block, ArrayRef<NamedAttribute> attrs);
+
+ /// Returns a list of AffineMap with the typical matmul indexing charactristic.
+ static SmallVector<AffineMap> getDefaultIndexingMaps(MLIRContext *context);
+
+ static std::function<void(ImplicitLocOpBuilder &,
+ Block &, ArrayRef<NamedAttribute>)>
+ getRegionBuilder() { return regionBuilder; }
+
+ ::mlir::MutableOperandRange getDpsInitsMutable() { return getOutputsMutable(); }
+
+ bool hasDynamicIndexingMaps() { return true; }
+
+ /// Returns the number of spatial dimensions, i.e. 1 for 1D convolution,
+ /// 2 for 2D convolution, etc.
+ int64_t getNumSpatialDims();
+
+ bool isDepthwise();
+ bool isGrouped();
+ bool isBatched();
+ }];
+}
+
//===----------------------------------------------------------------------===//
// Named Linalg ops, implemented as a declarative configurations of generic ops.
//===----------------------------------------------------------------------===//
diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
index 8973e87c063b33..03d9a7f3f09ce3 100644
--- a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
+++ b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
@@ -203,6 +203,41 @@ static void buildMatmulOp(OpBuilder &b, OperationState &state,
attributes, regionBuilder);
}
+static void buildConvOp(OpBuilder &b, OperationState &state,
+ std::optional<TypeRange> resultTensorTypes,
+ ValueRange inputs, ValueRange outputs,
+ ConvDimsAttr inputDims, ConvDimsAttr filterDims,
+ ConvDimsAttr outputDims, Attribute strides,
+ Attribute dilations,
+ ArrayRef<NamedAttribute> attributes,
+ RegionBuilderFn regionBuilder) {
+ state.addAttribute("input_dims", inputDims);
+ state.addAttribute("filter_dims", filterDims);
+ state.addAttribute("output_dims", outputDims);
+ if (strides)
+ state.addAttribute("strides", strides);
+
+ if (dilations)
+ state.addAttribute("dilations", dilations);
+ return buildStructuredOp(b, state, resultTensorTypes, inputs, outputs,
+ attributes, regionBuilder);
+}
+
+static void buildConvOp(OpBuilder &b, OperationState &state,
+ std::optional<TypeRange> resultTensorTypes, Value input,
+ Value filter, Value output, ConvDims inputDims,
+ ConvDims filterDims, ConvDims outputDims,
+ ArrayRef<int64_t> strides, ArrayRef<int64_t> dilations,
+ ArrayRef<NamedAttribute> attributes,
+ RegionBuilderFn regionBuilder) {
+ auto iAttr = ConvDimsAttr::get(b.getContext(), inputDims);
+ auto fAttr = ConvDimsAttr::get(b.getContext(), filterDims);
+ auto oAttr = ConvDimsAttr::get(b.getContext(), outputDims);
+ return buildConvOp(b, state, resultTensorTypes, {input, filter}, {output},
+ iAttr, fAttr, oAttr, b.getI64VectorAttr(strides),
+ b.getI64VectorAttr(dilations), attributes, regionBuilder);
+}
+
/// Common parsing used for both named structured ops created by ods-gen and by
/// manually defined C++ ops. Does not handle regions.
static ParseResult
@@ -3611,5 +3646,216 @@ Speculation::Speculatability MatmulOp::getSpeculatability() {
return getGenericSpeculatabilityImpl(cast<LinalgOp>(getOperation()));
}
+//===----------------------------------------------------------------------===//
+// ConvOp
+//===----------------------------------------------------------------------===//
+
+bool ConvOp::isDepthwise() {
+ return !getFilterDims().contains(ConvDimEnum::INPUT_CHANNEL);
+}
+
+bool ConvOp::isGrouped() {
+ // If not all tensors contain the GROUP dimension, then it's either not a
+ // grouped convolution, or the number of groups is 1, which we also don't
+ // consider grouped.
+ return getInputDims().contains(ConvDimEnum::GROUP) &&
+ getFilterDims().contains(ConvDimEnum::GROUP) &&
+ getOutputDims().contains(ConvDimEnum::GROUP);
+}
+
+bool ConvOp::isBatched() {
+ // Both input and output tensors must contain the BATCH dimension.
+ return getInputDims().contains(ConvDimEnum::BATCH) &&
+ getOutputDims().contains(ConvDimEnum::BATCH);
+}
+
+int64_t ConvOp::getNumSpatialDims() {
+ if (getInputDims().contains(ConvDimEnum::SPATIAL_2))
+ return 3;
+ if (getInputDims().contains(ConvDimEnum::SPATIAL_1))
+ return 2;
+ return 1;
+}
+
+SmallVector<utils::IteratorType> ConvOp::getIteratorTypesArray() {
+ int numParallelDims = getOutputDims().size();
+
+ int numReductionDims = getNumSpatialDims();
+ if (!isDepthwise())
+ ++numReductionDims; // input channel
+
+ SmallVector<utils::IteratorType> iteratorTypes(numParallelDims,
+ utils::IteratorType::parallel);
+ iteratorTypes.append(numReductionDims, utils::IteratorType::reduction);
+ return iteratorTypes;
+}
+
+ArrayAttr ConvOp::getIndexingMaps() {
+ ArrayAttr cached = getOperation()->getAttrOfType<ArrayAttr>(
+ LinalgDialect::kMemoizedIndexingMapsAttrName);
+ if (cached)
+ return cached;
+
+ Builder b(getContext());
+ SmallVector<AffineExpr> strides, dilations;
+ {
+ SmallVector<int64_t> strideValues, dilationValues;
+
+ if (getStrides())
+ strideValues = SmallVector<int64_t>(getStrides()->getValues<int64_t>());
+ else
+ strideValues = SmallVector<int64_t>(getNumSpatialDims(), 1);
+
+ if (getDilations())
+ dilationValues =
+ SmallVector<int64_t>(getDilations()->getValues<int64_t>());
+ else
+ dilationValues = SmallVector<int64_t>(getNumSpatialDims(), 1);
+
+ for (int j = 0; j < getNumSpatialDims(); ++j) {
+ strides.push_back(b.getAffineConstantExpr(strideValues[j]));
+ dilations.push_back(b.getAffineConstantExpr(dilationValues[j]));
+ }
+ }
+
+ llvm::DenseMap<ConvDimEnum, AffineExpr> parallelDims;
+ llvm::DenseMap<ConvDimEnum, AffineExpr> reductionDims;
+ SmallVector<AffineExpr> oExprs;
+
+ // Via the iterator types, we have defined the parallel loops to come first,
+ // followed by the reduction loops. We choose the order of the parallel loops
+ // to match the order of the output tensor dimensions. This is arbitrary and
+ // is done to follow the convention which most/some of the old linalg
+ // convolution ops follow.
+ int64_t i = 0;
+ for (auto d : getOutputDims()) {
+ auto expr = b.getAffineDimExpr(i++);
+ parallelDims[d] = expr;
+ oExprs.push_back(expr);
+ }
+ // Reduction loops are ordered to match the order of the filter tensor.
+ for (auto d : getFilterDims())
+ if (d == ConvDimEnum::INPUT_CHANNEL || d == ConvDimEnum::SPATIAL_0 ||
+ d == ConvDimEnum::SPATIAL_1 || d == ConvDimEnum::SPATIAL_2)
+ reductionDims[d] = b.getAffineDimExpr(i++);
+
+ SmallVector<AffineExpr> iExprs =
+ llvm::map_to_vector(getInputDims(), [&](ConvDimEnum dim) -> AffineExpr {
+ switch (dim) {
+ case ConvDimEnum::SPATIAL_0:
+ return (parallelDims[dim] * strides[0]) +
+ (reductionDims[dim] * dilations[0]);
+ case ConvDimEnum::SPATIAL_1:
+ return (parallelDims[dim] * strides[1]) +
+ (reductionDims[dim] * dilations[1]);
+ case ConvDimEnum::SPATIAL_2:
+ return (parallelDims[dim] * strides[2]) +
+ (reductionDims[dim] * dilations[2]);
+ case ConvDimEnum::INPUT_CHANNEL:
+ return reductionDims[dim];
+ default:
+ return parallelDims[dim];
+ }
+ });
+ SmallVector<AffineExpr> fExprs =
+ llvm::map_to_vector(getFilterDims(), [&](ConvDimEnum dim) -> AffineExpr {
+ if (reductionDims.contains(dim))
+ return reductionDims[dim];
+ return parallelDims[dim];
+ });
+
+ cached = b.getAffineMapArrayAttr(
+ {AffineMap::get(getNumLoops(), 0, iExprs, getContext()),
+ AffineMap::get(getNumLoops(), 0, fExprs, getContext()),
+ AffineMap::get(getNumLoops(), 0, oExprs, getContext())});
+ getOperation()->setAttr(LinalgDialect::kMemoizedIndexingMapsAttrName, cached);
+ return cached;
+}
+
+void ConvOp::regionBuilder(ImplicitLocOpBuilder &b, Block &block,
+ ArrayRef<NamedAttribute> attrs) {
+ RegionBuilderHelper helper(b, block);
+ SmallVector<Value> yields;
+
+ TypeFn castVal = TypeFn::cast_signed;
+ auto castIter = llvm::find_if(attrs, [&](const NamedAttribute &attr) {
+ return attr.getName() == "cast";
+ });
+ if (castIter != attrs.end()) {
+ if (auto attr = llvm::dyn_cast<TypeFnAttr>(castIter->getValue()))
+ castVal = attr.getValue();
+ }
+
+ Value value1 = helper.buildTypeFn(castVal, block.getArgument(2).getType(),
+ block.getArgument(0));
+ Value value2 = helper.buildTypeFn(castVal, block.getArgument(2).getType(),
+ block.getArgument(1));
+ Value value3 = helper.buildBinaryFn(BinaryFn::mul, value1, value2);
+ Value value4 =
+ helper.buildBinaryFn(BinaryFn::add, block.getArgument(2), value3);
+ yields.push_back(value4);
+ helper.yieldOutputs(yields);
+}
+
+ParseResult ConvOp::parse(OpAsmParser &parser, OperationState &result) {
+ return ::parseNamedStructuredOp(parser, result, 3,
+ ConvOp::getRegionBuilder());
+}
+void ConvOp::print(OpAsmPrinter &p) {
+ SmallVector<StringRef, 3> elidedAttrs = {"operandSegmentSizes",
+ "linalg.memoized_indexing_maps"};
+ ::printNamedStructuredOp(p, getOperation(), getInputs(), getOutputs(),
+ elidedAttrs);
+}
+
+LogicalResult ConvOp::verify() {
+ // Batch dimension cannot be present in filter tensor.
+ if (getFilterDims().contains(ConvDimEnum::BATCH))
+ return emitOpError("Batch dimension cannot be present in filter tensor.");
+
+ // Output channel cannot be present in input tensor.
+ if (getInputDims().contains(ConvDimEnum::OUTPUT_CHANNEL))
+ return emitOpError("Output channel cannot be present in input tensor.");
+
+ // Higher space dimensions cannot occur without the respective lower ones, so
+ // as to work with the `strides` and `dilations` attributes.
+ bool isSpat2 = getInputDims().contains(ConvDimEnum::SPATIAL_2);
+ bool isSpat1 = getInputDims().contains(ConvDimEnum::SPATIAL_1);
+ bool isSpat0 = getInputDims().contains(ConvDimEnum::SPATIAL_0);
+
+ if ((isSpat2 && (!isSpat1 || !isSpat0)) || (isSpat1 && !isSpat0))
+ return emitOpError("Inconsistent spatial dimensions in `input_dims`.");
+
+ if (!isSpat0)
+ return emitOpError("Requires at least one spatial dimension.");
+
+ // Spatial dimensions have to match between all tensors.
+ if (isSpat2 != getFilterDims().contains(ConvDimEnum::SPATIAL_2) ||
+ isSpat2 != getOutputDims().contains(ConvDimEnum::SPATIAL_2) ||
+ isSpat1 != getFilterDims().contains(ConvDimEnum::SPATIAL_1) ||
+ isSpat1 != getOutputDims().contains(ConvDimEnum::SPATIAL_1) ||
+ isSpat0 != getFilterDims().contains(ConvDimEnum::SPATIAL_0) ||
+ isSpat0 != getOutputDims().contains(ConvDimEnum::SPATIAL...
[truncated]
|
rengolin
requested review from
ftynse,
MaheshRavishankar,
javedabsar1 and
rolfmorel
|
Thanks, I'll copy the description of the current state from the discours thread here for reference: Currently, we have a proposed linalg.conv op using the above mentioned principle of 4 named dimensions + up to 3 spatial dimensions. We can also convert the old (non-quantized) convolution ops to the new one. The different “subtypes” of convolution emerge only from the dimensions of the tensors and can be queried via e.g. ConvOp::isDepthwise() and ConvOps::getSpatialDims(). “Quantized” (i.e. equivalent to the old ..._q ops is not implemented yet but could be implemented via a separate op or via an additional operand. There’s a lot of room for improvement, e.g. expanding the number of possible spatial dimensions from 3 to N, and the ConvDimsAttr assembly format. But I’d like to see how happy people are with the general idea of this op. |
|
In the original thread, there were a lot of different views. Because this is not a trivial change, we need to make sure all those views were taken into account. It's not clear that it has, since the people involved in the original discussion were not aware of this PR. I welcome and encourage the enthusiasm of working in various areas of LLVM, but we should not start pushing code upstream that has not been shown to go in the right direction. That'll need buy-in from all parties. I'm hoping we can use this PR to hash that out, but the number of missing things and "room for improvement" tells me it will not be easy. In the future, for large changes like this, I strongly encourage you to work with the people who expressed concerns in the RFC before you push a PR. It saves us all the trouble of yet-another discussion that has no conclusion. |
|
This PR is the direct result of the discourse thread, but there is a reason for why discussion in that threa died down. It's an abstract topic, some "expressed views" are just mutually exclusive and at some point, people need to see how it would look in practice. The goal of this PR is to give the participants a working principle for this new Op and an opportunity to specify exactly how it would need to change for it to work for them. Like you said, the goal is to get buy-in of all parties on the right direction, not proposing the end-all-be-all solution for convolution in linalg. |
It didn't die down, we're all taking hold of the discussed points and understanding how they affect our own work. If you think a thread has died down, the easiest thing to do is just ping the thread asking if it did.
That's a worthy goal, but without direction, it could make it harder for all parties to agree. All I'm asking is that you work with the affected people to put a proposal out. It has a much higher chance of avoiding endless discussions and does not detract the discussion from upstream.
That's not what I said. No one wants to go through this again. We want buy-in for the final solution. We already have "buy-in" for the wrong solution, and that's what we've been trying to correct. Less haste, more speed. |
|
I agree with the points raised by Renato.
While I appreciate the initiative, there was a Call for Action in the thread that remained mostly unanswered. This PR feels a bit unexpected as a result. For core logic like this, it's crucial to coordinate work in the original thread to ensure alignment. In particular, Mahesh proposed a plan, and it's natural to assume he would like to follow through on it. Have you had the chance to discuss this with him?
There’s been excellent feedback, and the next step involves proposing a refined design to address the concerns raised. This is a complex problem, and developing a solution that incorporates all the feedback takes time and thoughtful consideration. Could you clarify how this PR aligns with the key points from the thread? One critical aspect missing here is alignment with the recent refactoring of That refactoring involved a thorough and detailed discussion to ensure future-proofing. Given the complexity and variety of Convs, achieving a similar state will likely require even more time and deliberation. Some delay is natural and expected. That said, this PR provides a valuable data point that will undoubtedly contribute to the ongoing discussion. Thank you for sharing this! I’m a bit constrained this week, so apologies in advance for any delays on my side. EDIT Fixed link to Mahesh's plan. |
I think the link is wrong, it doesn't point to a plan of Mahesh's. The plan of his that I am aware of is this: #113953 (comment) which lays out different options of how to deal with the addition of new convolution op variants until the "real" op (which this PR proposes) comes in. |
That's not a plan, just a comment. This is the problem of picking up comments on threads on RFCs and PRs. We were all talking the same things in those threads, but we had some key differences. We need to distill the differences and make a solid design plan. This design should not come from a single person on a PR to "foster discussion", it needs to be discussed on the merits of each delta, otherwise we're back to square one trying to fix convolutions again. This is not the first time we try this, and it's the reason why I'm reluctant to invest time (again) in designing an operation without actual data from people that are actually using it in production. |
| // CHECK: } | ||
| // CHECK: } | ||
| func.func @conv_1d_ncw_fcw(%input: tensor<?x?x?xf32>, %filter: tensor<?x?x?xf32>, %init: tensor<?x?x?xf32>) -> tensor<?x?x?xf32> { | ||
| %0 = linalg.conv_1d_ncw_fcw {dilations = dense<1> : tensor<1xi64>, |
There was a problem hiding this comment.
These are the old conv ops, which the PR does not change, right? Why are they being tested (and not the new linalg.conv)?
There was a problem hiding this comment.
This test is designed to make sure that we don't lose the possibility to represent any of the "old" ops, and also to make sure that the generalization of the resulting new op leads to the correct generic representation. The -test-linalg-new-conv pass in the test pipeline first converts the old op to the new op. Then the result is generalized and compared to an expected result.
We should definitely have more fine-grained tests in the future as well - this one is an end-to-end test in an attempt to verify the compatibility to the old ops.
It was wrong, apologies for that. Fixed. |
| ConvDimEnumAttrCase<"GROUP", 3, "G">, | ||
| /// Spatial dimensions occur in all tensors. Output is indexed from a parallel | ||
| /// loop, filter from a reduction loop and input from both. | ||
| ConvDimEnumAttrCase<"SPATIAL_0", 4, "0">, |
There was a problem hiding this comment.
I'm still on team "just make this an arbitrarily-large integer" because there's no actual reason to stop at 3 here, and stuff'll break if someone ever needs a 4D convolution
There was a problem hiding this comment.
Agreed, the reason it's not in this PR is just so keep it representable as a simple enum for now and to keep everything testable via the old ops, which only went up to 3d. But the denotion spatial_0, ... was chosen to be extended to N dimensions via a more tailored attribute.
|
Commenting quickly while I am on vacation. I'll be back Monday. |
This patch lays the groundwork for the new
linalg.convop which is designed to replace the multitude oflinalg.conv_...as well aslinalg.depthwise_conv_...ops.A test pass is implemented which can convert the old conv ops to the new op. The
linalg-generalize-named-opscan then be used to convert both the old and the new ops to alinalg.genericop for comparison.