[Mlir-commits] [mlir] fe5370d - [mlir] Store segment sizes in std::array

[Tobias Gysi]

Author: Tobias Gysi
Date: 2023-08-03T08:29:16Z
New Revision: fe5370dd50e48f827aaee9f5fa80e751b1759a25

URL: https://github.com/llvm/llvm-project/commit/fe5370dd50e48f827aaee9f5fa80e751b1759a25
DIFF: https://github.com/llvm/llvm-project/commit/fe5370dd50e48f827aaee9f5fa80e751b1759a25.diff

LOG: [mlir] Store segment sizes in std::array

This revision uses std::array instead of normal c arrays to store the
operand and result segment sizes. This is a follow up to
https://reviews.llvm.org/D155919, which converted the operand and result
segment sizes to properties. Its use of c arrays triggered warnings in
downstream projects due to the direct comparison of c arrays. This
revision fixes the warnings using std::arrays that implement a
proper comparison operator, which compares the array elements rather
that the array pointers.

Note: it seems the comparison operator is effectively dead code for now.
It still seems useful to fix the warning and ensure the comparison works
as expected assume someone starts using it at some point in time.

Reviewed By: mehdi_amini

Differential Revision: https://reviews.llvm.org/D156888

Added: 
    

Modified: 
    mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
    mlir/tools/mlir-tblgen/OpFormatGen.cpp
    mlir/unittests/IR/AdaptorTest.cpp

Removed: 
    


################################################################################
diff  --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 7da61ff991ec58..44cb61a93b545d 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -164,7 +164,7 @@ if ($_reader.getBytecodeVersion() < /*kNativePropertiesODSSegmentSize=*/6) {
     $_reader.emitError("size mismatch for operand/result_segment_size");
     return failure();
   }
-  llvm::copy(ArrayRef<int32_t>(attr), $_storage);
+  llvm::copy(ArrayRef<int32_t>(attr), $_storage.begin());
 } else {
   return $_reader.readSparseArray(MutableArrayRef($_storage));
 }
@@ -429,7 +429,8 @@ void OpOrAdaptorHelper::computeAttrMetadata() {
         /*storageType=*/storageType,
         /*interfaceType=*/"::llvm::ArrayRef<int32_t>",
         /*convertFromStorageCall=*/"$_storage",
-        /*assignToStorageCall=*/"::llvm::copy($_value, $_storage)",
+        /*assignToStorageCall=*/
+        "llvm::copy($_value, $_storage.begin())",
         /*convertToAttributeCall=*/
         "DenseI32ArrayAttr::get($_ctxt, $_storage)",
         /*convertFromAttributeCall=*/
@@ -445,7 +446,7 @@ void OpOrAdaptorHelper::computeAttrMetadata() {
   if (op.getTrait("::mlir::OpTrait::AttrSizedOperandSegments")) {
     if (op.getDialect().usePropertiesForAttributes()) {
       operandSegmentsSizeStorage =
-          llvm::formatv("int32_t[{0}]", op.getNumOperands());
+          llvm::formatv("std::array<int32_t, {0}>", op.getNumOperands());
       operandSegmentsSize = {"odsOperandSegmentSizes",
                              makeProperty(operandSegmentsSizeStorage)};
     } else {
@@ -458,7 +459,7 @@ void OpOrAdaptorHelper::computeAttrMetadata() {
   if (op.getTrait("::mlir::OpTrait::AttrSizedResultSegments")) {
     if (op.getDialect().usePropertiesForAttributes()) {
       resultSegmentsSizeStorage =
-          llvm::formatv("int32_t[{0}]", op.getNumResults());
+          llvm::formatv("std::array<int32_t, {0}>", op.getNumResults());
       resultSegmentsSize = {"odsResultSegmentSizes",
                             makeProperty(resultSegmentsSizeStorage)};
     } else {
@@ -1491,7 +1492,7 @@ void OpEmitter::genPropertiesSupport() {
        if (!arrAttr) return;
        if (arrAttr.size() != sizeof(prop.{0}) / sizeof(int32_t))
          return;
-       llvm::copy(arrAttr.asArrayRef(), prop.{0});
+       llvm::copy(arrAttr.asArrayRef(), prop.{0}.begin());
        return;
     }
 )decl",
@@ -2341,7 +2342,8 @@ void OpEmitter::genSeparateArgParamBuilder() {
             });
         if (op.getDialect().usePropertiesForAttributes()) {
           body << "}), " << builderOpState
-               << ".getOrAddProperties<Properties>().odsResultSegmentSizes);\n";
+               << ".getOrAddProperties<Properties>()."
+                  "odsResultSegmentSizes.begin());\n";
         } else {
           body << "}));\n";
         }
@@ -2966,7 +2968,8 @@ void OpEmitter::genCodeForAddingArgAndRegionForBuilder(
       body << "  llvm::copy(ArrayRef<int32_t>({";
       emitSegment();
       body << "}), " << builderOpState
-           << ".getOrAddProperties<Properties>().odsOperandSegmentSizes);\n";
+           << ".getOrAddProperties<Properties>()."
+              "odsOperandSegmentSizes.begin());\n";
     } else {
       body << "  " << builderOpState << ".addAttribute(" << sizes << "AttrName("
            << builderOpState << ".name), "

diff  --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index 2efd82fd526dbf..c38f873ddaba4c 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -1666,7 +1666,7 @@ void OperationFormat::genParserVariadicSegmentResolution(Operator &op,
         llvm::interleaveComma(op.getOperands(), body, interleaveFn);
         body << formatv("}), "
                         "result.getOrAddProperties<{0}::Properties>()."
-                        "odsOperandSegmentSizes);\n",
+                        "odsOperandSegmentSizes.begin());\n",
                         op.getCppClassName());
       } else {
         body << "  result.addAttribute(\"operand_segment_sizes\", "
@@ -1708,11 +1708,10 @@ void OperationFormat::genParserVariadicSegmentResolution(Operator &op,
     if (op.getDialect().usePropertiesForAttributes()) {
       body << "llvm::copy(ArrayRef<int32_t>({";
       llvm::interleaveComma(op.getResults(), body, interleaveFn);
-      body << formatv(
-          "}), "
-          "result.getOrAddProperties<{0}::Properties>().odsResultSegmentSizes"
-          ");\n",
-          op.getCppClassName());
+      body << formatv("}), "
+                      "result.getOrAddProperties<{0}::Properties>()."
+                      "odsResultSegmentSizes.begin());\n",
+                      op.getCppClassName());
     } else {
       body << "  result.addAttribute(\"result_segment_sizes\", "
            << "parser.getBuilder().getDenseI32ArrayAttr({";

diff  --git a/mlir/unittests/IR/AdaptorTest.cpp b/mlir/unittests/IR/AdaptorTest.cpp
index aae8c1ac44dbf7..4d1d49f56c8cd6 100644
--- a/mlir/unittests/IR/AdaptorTest.cpp
+++ b/mlir/unittests/IR/AdaptorTest.cpp
@@ -40,12 +40,18 @@ TEST(Adaptor, GenericAdaptorsOperandAccess) {
     // value from the value 0.
     SmallVector<std::optional<int>> v = {0, 4};
     OIListSimple::Properties prop;
-    llvm::copy(ArrayRef{1, 0, 1}, prop.odsOperandSegmentSizes);
+    prop.odsOperandSegmentSizes = {1, 0, 1};
     OIListSimple::GenericAdaptor<ArrayRef<std::optional<int>>> d(v, {}, prop,
                                                                  {});
     EXPECT_EQ(d.getArg0(), 0);
     EXPECT_EQ(d.getArg1(), std::nullopt);
     EXPECT_EQ(d.getArg2(), 4);
+
+    // Check the property comparison operator.
+    OIListSimple::Properties equivalentProp = {1, 0, 1};
+    OIListSimple::Properties 
diff erentProp = {0, 0, 1};
+    EXPECT_EQ(d.getProperties(), equivalentProp);
+    EXPECT_NE(d.getProperties(), 
diff erentProp);
   }
 
   // Has VariadicOfVariadic arguments.