[mlir][flang] Do not prevent integer types from being parsed as MLIR keywords

[jeanPerier]

Summary

DialectAsmParser::parseKeyword is rejecting 'i' digit+ while it is
a valid identifier according to mlir/docs/LangRef.md.

Integer types actually used to be TOK_KEYWORD a while back before the
change: https://github.com/llvm/llvm-project/commit/6af866c58d21813fb243906611d02bb2a8ffa43a.

This patch Modifies isCurrentTokenAKeyword to return true for tokens that
match integer types too.

The motivation for this change is the parsing of !fir.type<{ component-name: component-type,+ }>
type in FIR that represent Fortran derived types. The component-names are
parsed as keywords, and can very well be i32 or any ixxx (which are
valid Fortran derived type component names).

I am not sure where to add a pure MLIR test for this change, so I added
one in FIR, although the functional change is in MLIR parser.

[ftynse]

Could we have a MLIR test for this?

[jeanPerier]

My patch actually breaks a bunch of Quant and Tosa regression tests because quant.uniform parser is assuming that integer types cannot be parsed as keywords here: https://github.com/llvm/llvm-project/blob/6bc767cd071ccdb41b5532f7d9cae22999e0fac4/mlir/lib/Dialect/Quant/IR/TypeParser.cpp#L32

So now I am wondering if I am following a wrong approach or if the quant parsing should be modified. Any opinions ?

[jeanPerier]

In D108913#2972124, @ftynse wrote:

Could we have a MLIR test for this?

How best would you suggest testing parseKeyword behavior with pure a MLIR test ?

When I grepped for negative keyword parsing tests ("expected valid keyword" error), I could only find tests with Linalg and SPIRV, so I am not sure how to come with a pure MLIR test that would show that ixxx are accepted as keywords.

[ftynse]

When I grepped for negative keyword parsing tests ("expected valid keyword" error), I could only find tests with Linalg and SPIRV, so I am not sure how to come with a pure MLIR test that would show that ixxx are accepted as keywords.

Create an op/type in the test dialect that uses keywords in its custom syntax and try parsing it.

[jeanPerier]

Add test with MLIR test dialect and change Quant dialect type parser
according to parseKeyword behaviour change (Fixes regressions).

[jeanPerier]

In D108913#2972236, @ftynse wrote:

When I grepped for negative keyword parsing tests ("expected valid keyword" error), I could only find tests with Linalg and SPIRV, so I am not sure how to come with a pure MLIR test that would show that ixxx are accepted as keywords.

Create an op/type in the test dialect that uses keywords in its custom syntax and try parsing it.

Thanks for the direction @ftynse , I have added a test with the MLIR test dialect struct type.

flang/test/Fir/fir-types.fir

 
 // Fortran Derived types (records)
 // CHECK-LABEL: func private @dvd1() -> !fir.type<derived1>
 // CHECK-LABEL: func private @dvd2() -> !fir.type<derived2(p:i32)>
 // CHECK-LABEL: func private @dvd3() -> !fir.type<derived3{f:f32}>
 // CHECK-LABEL: func private @dvd4() -> !fir.type<derived4(p:i8){f:f32}>
 // CHECK-LABEL: func private @dvd5() -> !fir.type<derived5(p1:i8,p2:i8,p3:i8,p4:i8,p5:i8){f1:f32,f2:f32,f3:f32,f4:f32,f5:f32,f6:f32,f7:f32,f8:f32}>
 // CHECK-LABEL: func private @dvd6() -> !fir.type<derived6{f:!fir.ptr<!fir.type<derived6>>}>
+// CHECK-LABEL: func private @dvd7() -> !fir.type<derived_with_field_name_same_as_integer_type{i32:f32}>
 func private @dvd1() -> !fir.type<derived1>
 func private @dvd2() -> !fir.type<derived2(p:i32)>
 func private @dvd3() -> !fir.type<derived3{f:f32}>
 func private @dvd4() -> !fir.type<derived4(p:i8){f:f32}>
 func private @dvd5() -> !fir.type<derived5(p1:i8,p2:i8,p3:i8,p4:i8,p5:i8){f1:f32,f2:f32,f3:f32,f4:f32,f5:f32,f6:f32,f7:f32,f8:f32}>
 func private @dvd6() -> !fir.type<derived6{f:!fir.ptr<!fir.type<derived6>>}>
+func private @dvd7() -> !fir.type<derived_with_field_name_same_as_integer_type{i32:f32}>
 
 // FIR array types
 // CHECK-LABEL: func private @arr1() -> !fir.array<10xf32>
 // CHECK-LABEL: func private @arr2() -> !fir.array<10x10xf32>
 // CHECK-LABEL: func private @arr3() -> !fir.array<?xf32>
 // CHECK-LABEL: func private @arr4() -> !fir.array<10x?xf32>
 // CHECK-LABEL: func private @arr5() -> !fir.array<?x?xf32>
 // CHECK-LABEL: func private @arr6() -> !fir.array<*:f32>

mlir/lib/Dialect/Quant/IR/TypeParser.cpp

 
 static IntegerType parseStorageType(DialectAsmParser &parser, bool &isSigned) {
   auto typeLoc = parser.getCurrentLocation();
   IntegerType type;
 
   // Parse storage type (alpha_ident, integer_literal).
   StringRef identifier;
   unsigned storageTypeWidth = 0;
-  if (failed(parser.parseOptionalKeyword(&identifier))) {
-    // If we didn't parse a keyword, this must be a signed type.
-    if (parser.parseType(type))
+  OptionalParseResult result = parser.parseOptionalType(type);
+  if (result.hasValue()) {
+    if (!succeeded(*result)) {
+      parser.parseType(type);
       return nullptr;
-    isSigned = true;
+    }
+    isSigned = !type.isUnsigned();
     storageTypeWidth = type.getWidth();
-
+  } else if (succeeded(parser.parseKeyword(&identifier))) {
     // Otherwise, this must be an unsigned integer (`u` integer-literal).
-  } else {
     if (!identifier.consume_front("u")) {
       parser.emitError(typeLoc, "illegal storage type prefix");
       return nullptr;
     }
     if (identifier.getAsInteger(10, storageTypeWidth)) {
       parser.emitError(typeLoc, "expected storage type width");
       return nullptr;
     }
     isSigned = false;
     type = parser.getBuilder().getIntegerType(storageTypeWidth);
+  } else {
+    return nullptr;
   }
 
   if (storageTypeWidth == 0 ||
       storageTypeWidth > QuantizedType::MaxStorageBits) {
     parser.emitError(typeLoc, "illegal storage type size: ")
         << storageTypeWidth;
     return nullptr;
   }

mlir/lib/Parser/DialectSymbolParser.cpp

ParseResult parseOptionalString(std::string *string) override {
     if (string)
       *string = parser.getToken().getStringValue();
     parser.consumeToken();
     return success();
   }
 
   /// Returns true if the current token corresponds to a keyword.
   bool isCurrentTokenAKeyword() const {
-    return parser.getToken().is(Token::bare_identifier) ||
+    return parser.getToken().isAny(Token::bare_identifier, Token::inttype) ||
            parser.getToken().isKeyword();
   }
 
   /// Parse the given keyword if present.
   ParseResult parseOptionalKeyword(StringRef keyword) override {
     // Check that the current token has the same spelling.
     if (!isCurrentTokenAKeyword() || parser.getTokenSpelling() != keyword)
       return failure();

mlir/test/mlir-tblgen/types.mlir

 
 // -----
 
 func @elements_attr_not_index() {
   // expected-error@+1 {{index elements attribute}}
   "test.indexElementsAttr"() {attr = dense<[1, 2]>:tensor<2xi32>} : () -> ()
   return
 }
+
+// -----
+
+// CHECK-LABEL: @struct_success
+func @struct_success() {
+  "test.simple_struct"() : () -> (!test.struct<{a, i32}, {b, f64}>)
+  return
+}
+
+// -----
+
+// CHECK-LABEL: @struct_with_field_names_like_types
+func @struct_with_field_names_like_types() {
+  "test.struct_with_field_names_like_types"() : () -> (!test.struct<{i32, i32}, {f64, f64}>)
+  return
+}
+
+// -----
+
+func @struct_bad_keywords() {
+  // expected-error@+1 {{expected valid keyword}}
+  "test.struct_bad_keywords"() : () -> (!test.struct<{42, i32}>)
+  return
+}