Fix GCC build problem with 288f05f related to SmallVector. #116958

shahidact · 2024-11-20T11:56:22Z

Below is the error message for reference.

/llvm-project/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp: In static member function 'static llvm::SmallVectormlir::AffineMap mlir::linalg::MatmulOp::getDefaultIndexingMaps(mlir::MLIRContext*)':
/llvm-project/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:3468:10: error: could not convert 'indexingMaps' from 'SmallVector<[...],3>' to 'SmallVector<[...],6>'
3468 | return indexingMaps;
| ^~~~~~~~~~~~
| |
| SmallVector<[...],3>

Here is the link to the failure.
https://lab.llvm.org/buildbot/#/builders/117/builds/3919
...

llvmbot · 2024-11-20T11:56:55Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-linalg

Author: Md Asghar Ahmad Shahid (shahidact)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/116958.diff

2 Files Affected:

(modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td (+7-13)
(modified) mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp (+31-50)

diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td b/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
index a90777c82bf63a..37eec6e07963b1 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgStructuredOps.td
@@ -622,7 +622,8 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
             CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
       [{
         buildMatmulOp($_builder, $_state, std::nullopt, inputs, outputs,
-          attributes, MatmulOp::getRegionBuilder());
+          attributes, MatmulOp::getRegionBuilder(),
+          MatmulOp::getDefaultIndexingMaps($_builder.getContext()));
       }]>,
       OpBuilder<
       (ins "TypeRange":$resultTensorTypes, "ValueRange":$inputs,
@@ -630,16 +631,8 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
             CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
       [{
         buildMatmulOp($_builder, $_state, resultTensorTypes,
-          inputs, outputs, attributes, MatmulOp::getRegionBuilder());
-      }]>,
-      OpBuilder<
-      (ins "TypeRange":$resultTensorTypes, "ValueRange":$operands,
-            CArg<"ArrayRef<NamedAttribute>", "{}">:$attributes),
-      [{
-        $_state.addOperands(operands);
-        $_state.addAttributes(attributes);
-        $_state.addTypes(resultTensorTypes);
-        (void)$_state.addRegion();
+          inputs, outputs, attributes, MatmulOp::getRegionBuilder(),
+          MatmulOp::getDefaultIndexingMaps($_builder.getContext()));
       }]>,
       OpBuilder<
       (ins "TypeRange":$resultTensorTypes, "ValueRange":$inputs,
@@ -648,7 +641,8 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
       [{
         $_state.addAttribute("cast", cast);
         buildMatmulOp($_builder, $_state, resultTensorTypes, inputs, outputs,
-          attributes, MatmulOp::getRegionBuilder());
+          attributes, MatmulOp::getRegionBuilder(),
+          MatmulOp::getDefaultIndexingMaps($_builder.getContext()));
       }]>
 
     ];
@@ -664,7 +658,7 @@ def MatmulOp : LinalgStructuredBase_Op<"matmul", [
                                 Block &block, ArrayRef<NamedAttribute> attrs);
 
       /// Returns a list of AffineMap with the typical matmul indexing charactristic.
-      SmallVector<AffineMap> getDefaultIndexingMaps();
+      static SmallVector<AffineMap> getDefaultIndexingMaps(MLIRContext *context);
 
       /// Returns true if the given broadcast map \p bcastMap is valid for this op.
       bool isValidLhsRhsBroadcastMap(AffineMap bcastMap);
diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
index dee8a4e27e6b26..26d9d2b091750c 100644
--- a/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
+++ b/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp
@@ -155,27 +155,6 @@ static void fillStructuredOpRegion(OpBuilder &opBuilder, Region &region,
   // iterator_types is an auto-generated method.
 }
 
-/// Helper to create a typical indexing map for MatmulOp. Returns a list of
-/// AffineMap.
-static SmallVector<AffineMap, 3>
-getDefaultIndexingMapsForMatmul(MLIRContext *context) {
-  AffineExpr d0, d1, d2;
-  SmallVector<AffineMap, 3> indexingMaps;
-  bindDims(context, d0, d1, d2);
-  indexingMaps.push_back(AffineMap::get(3, 0, {d0, d2}, context));
-  indexingMaps.push_back(AffineMap::get(3, 0, {d2, d1}, context));
-  indexingMaps.push_back(AffineMap::get(3, 0, {d0, d1}, context));
-  return indexingMaps;
-}
-
-/// Wrapper to return the typical indexing map array attribute for MatmulOp.
-static SmallVector<Attribute>
-getDefaultMatmulIndexingMapAttr(MLIRContext *context) {
-  return llvm::map_to_vector(
-      getDefaultIndexingMapsForMatmul(context),
-      [](AffineMap map) -> Attribute { return AffineMapAttr::get(map); });
-}
-
 /// Creates a structured operation given `inputs`, `outputs`, and `attributes`.
 /// The result types are derived automatically if `resultTensorTypes` is none.
 /// The body of the operation is filled using `regionBuilder`. All ods-gen
@@ -208,24 +187,18 @@ static void buildStructuredOp(OpBuilder &b, OperationState &state,
                          state.attributes.getAttrs(), regionBuilder);
 }
 
-static void
-buildMatmulOp(OpBuilder &b, OperationState &state,
-              std::optional<TypeRange> resultTensorTypes, ValueRange inputs,
-              ValueRange outputs, ArrayRef<NamedAttribute> attributes,
-              RegionBuilderFn regionBuilder,
-              std::optional<ArrayRef<AffineMap>> indexingMaps = std::nullopt) {
-  // Initialize indexingMaps, for MatmulOp.
+static void buildMatmulOp(OpBuilder &b, OperationState &state,
+                          std::optional<TypeRange> resultTensorTypes,
+                          ValueRange inputs, ValueRange outputs,
+                          ArrayRef<NamedAttribute> attributes,
+                          RegionBuilderFn regionBuilder,
+                          ArrayRef<AffineMap> indexingMaps) {
+  // Initialize indexingMaps attribute, for MatmulOp.
   SmallVector<Attribute, 3> indexingMapsAttrVal;
-  if (indexingMaps.has_value()) {
-    for (mlir::AffineMap map : *indexingMaps) {
-      // Convert each AffineMap to an AffineMapAttr
-      indexingMapsAttrVal.push_back(AffineMapAttr::get(map));
-    }
-    state.addAttribute("indexing_maps", b.getArrayAttr(indexingMapsAttrVal));
-  } else {
-    indexingMapsAttrVal = getDefaultMatmulIndexingMapAttr(b.getContext());
-    state.addAttribute("indexing_maps", b.getArrayAttr(indexingMapsAttrVal));
-  }
+  indexingMapsAttrVal = llvm::map_to_vector(
+      MatmulOp::getDefaultIndexingMaps(b.getContext()),
+      [](AffineMap map) -> Attribute { return AffineMapAttr::get(map); });
+  state.addAttribute("indexing_maps", b.getArrayAttr(indexingMapsAttrVal));
   return buildStructuredOp(b, state, resultTensorTypes, inputs, outputs,
                            attributes, regionBuilder);
 }
@@ -3457,7 +3430,7 @@ static LogicalResult verifyExtendedMatmulSemantic(MatmulOp matmulOp,
                                                   unsigned opIndex) {
   SmallVector<AffineMap, 3> opIndexingMaps = matmulOp.getIndexingMapsArray();
   SmallVector<AffineMap, 3> defaultIndexingMaps =
-      matmulOp.getDefaultIndexingMaps();
+      matmulOp.getDefaultIndexingMaps(matmulOp->getContext());
 
   auto opIndexingMap = opIndexingMaps[opIndex];
   auto defaultIndexingMap = defaultIndexingMaps[opIndex];
@@ -3484,6 +3457,17 @@ namespace linalg {
 // MatMulOp
 //===----------------------------------------------------------------------===//
 
+/// Returns a list of AffineMap with the typical matmul indexing charactristic.
+SmallVector<AffineMap> MatmulOp::getDefaultIndexingMaps(MLIRContext *context) {
+  AffineExpr d0, d1, d2;
+  SmallVector<AffineMap> indexingMaps;
+  bindDims(context, d0, d1, d2);
+  indexingMaps.push_back(AffineMap::get(3, 0, {d0, d2}, context));
+  indexingMaps.push_back(AffineMap::get(3, 0, {d2, d1}, context));
+  indexingMaps.push_back(AffineMap::get(3, 0, {d0, d1}, context));
+  return indexingMaps;
+}
+
 SmallVector<utils::IteratorType> MatmulOp::getIteratorTypesArray() {
   return SmallVector<utils::IteratorType>{utils::IteratorType::parallel,
                                           utils::IteratorType::parallel,
@@ -3501,7 +3485,8 @@ bool MatmulOp::hasDynamicIndexingMaps() { return true; }
 /// Check if the op has broadcast and/or transpose semantic. Returns true if
 /// the user defined indexing maps are not equal to default map.
 bool MatmulOp::hasUserDefinedMaps() {
-  SmallVector<AffineMap, 3> defaultMaps = getDefaultIndexingMaps();
+  SmallVector<AffineMap, 3> defaultMaps =
+      getDefaultIndexingMaps(this->getContext());
   SmallVector<AffineMap, 3> explicitMaps = getIndexingMapsArray();
   return defaultMaps != explicitMaps;
 }
@@ -3535,13 +3520,6 @@ void MatmulOp::regionBuilder(ImplicitLocOpBuilder &b, Block &block,
   helper.yieldOutputs(yields);
 }
 
-/// Returns a list of AffineMap with the typical matmul indexing
-/// charactristic.
-SmallVector<AffineMap> MatmulOp::getDefaultIndexingMaps() {
-  MLIRContext *context = this->getContext();
-  return getDefaultIndexingMapsForMatmul(context);
-}
-
 /// Returns true if the given broadcast map \p bcastMap is valid for this op.
 bool MatmulOp::isValidLhsRhsBroadcastMap(AffineMap bcastMap) {
   assert(bcastMap.getNumResults() == 1 && "Expected single result dim expr.");
@@ -3578,7 +3556,9 @@ ParseResult MatmulOp::parse(OpAsmParser &parser, OperationState &result) {
   }
   // Initialize indexingMaps, if not supplied explicitly.
   if (indexingMapsAttr.empty()) {
-    indexingMapsAttr = getDefaultMatmulIndexingMapAttr(result.getContext());
+    indexingMapsAttr = llvm::map_to_vector(
+        MatmulOp::getDefaultIndexingMaps(parser.getContext()),
+        [](AffineMap map) -> Attribute { return AffineMapAttr::get(map); });
   }
   result.addAttribute("indexing_maps",
                       parser.getBuilder().getArrayAttr(indexingMapsAttr));
@@ -3592,8 +3572,9 @@ void MatmulOp::print(OpAsmPrinter &p) {
   printNamedStructuredOp(p, getOperation(), getInputs(), getOutputs(),
                          elidedAttrs);
 
-  SmallVector<Attribute, 3> indexingMaps =
-      getDefaultMatmulIndexingMapAttr(getContext());
+  SmallVector<Attribute, 3> indexingMaps = llvm::map_to_vector(
+      MatmulOp::getDefaultIndexingMaps(getContext()),
+      [](AffineMap map) -> Attribute { return AffineMapAttr::get(map); });
   if (!llvm::equal(getIndexingMaps(), indexingMaps)) {
     p << " indexing_maps = [";
     llvm::interleaveComma(getIndexingMaps(), p,

rengolin

Same as before, thanks!

banach-space

LGTM, thanks!

Just a small nit :) Dumping a full log from your shell into a Git commit summary can make it hard to parse and less helpful. Could you make it a bit more concise and selective? For example, IMHO, this trimmed version of your log would be sufficient:

/llvm-project/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:3468:10: error: could not convert 'indexingMaps' from 'SmallVector<[...],3>' to 'SmallVector<[...],6>'
3468 | return indexingMaps;
| ^~~~~~~~~~~~
| |
| SmallVector<[...],3>

Including a link to the failing bot and the GCC version where this occurs would also be very helpful for context.

Lastly, if no bot is currently failing, it might indicate a gap in our testing. Do you think we should add coverage for this scenario?

shahidact · 2024-11-20T13:12:42Z

LGTM, thanks!

Just a small nit :) Dumping a full log from your shell into a Git commit summary can make it hard to parse and less helpful. Could you make it a bit more concise and selective? For example, IMHO, this trimmed version of your log would be sufficient:
/llvm-project/mlir/lib/Dialect/Linalg/IR/LinalgOps.cpp:3468:10: error: could not convert 'indexingMaps' from 'SmallVector<[...],3>' to 'SmallVector<[...],6>'
3468 | return indexingMaps;
| ^~~~~~~~~~~~
| |
| SmallVector<[...],3>
Including a link to the failing bot and the GCC version where this occurs would also be very helpful for context.

Lastly, if no bot is currently failing, it might indicate a gap in our testing. Do you think we should add coverage for this scenario?

Thanks. Yes the failure came from a bot. I will update its link in description.

kparzysz · 2024-11-20T13:41:34Z

This is a blocking breakage. I'm merging this, since it's been approved.

shahidact requested review from dcaballe, nicolasvasilache and rengolin as code owners November 20, 2024 11:56

llvmbot added mlir:linalg mlir labels Nov 20, 2024

Fix GCC build problem with 288f05f related to SmallVector

7f764aa

shahidact force-pushed the nfc-matmul branch from b1657b7 to 7f764aa Compare November 20, 2024 12:12

rengolin approved these changes Nov 20, 2024

View reviewed changes

shahidact mentioned this pull request Nov 20, 2024

[NFC][MLIR][Linalg] Refactor linalg.matmul tablegen ODS and related C++ code. #116377

Merged

banach-space approved these changes Nov 20, 2024

View reviewed changes

kparzysz merged commit d0d726e into llvm:main Nov 20, 2024
8 checks passed

shahidact mentioned this pull request Nov 20, 2024

mlir/LinalgOps.cpp fails to build with error: could not convert 'indexingMaps' from 'SmallVector<[...],3>' to 'SmallVector<[...],6>' #116966

Closed

pearu mentioned this pull request Nov 22, 2024

LLVM integration llvm/llvm-project@33fcd6acc755 is broken openxla/stablehlo#2637

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix GCC build problem with 288f05f related to SmallVector. #116958

Fix GCC build problem with 288f05f related to SmallVector. #116958

Uh oh!

shahidact commented Nov 20, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 20, 2024 •

edited

Loading

Uh oh!

rengolin left a comment

Uh oh!

banach-space left a comment

Uh oh!

shahidact commented Nov 20, 2024

Uh oh!

Uh oh!

kparzysz commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Fix GCC build problem with 288f05f related to SmallVector. #116958

Fix GCC build problem with 288f05f related to SmallVector. #116958

Uh oh!

Conversation

shahidact commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rengolin left a comment

Choose a reason for hiding this comment

Uh oh!

banach-space left a comment

Choose a reason for hiding this comment

Uh oh!

shahidact commented Nov 20, 2024

Uh oh!

Uh oh!

kparzysz commented Nov 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shahidact commented Nov 20, 2024 •

edited

Loading

llvmbot commented Nov 20, 2024 •

edited

Loading