[mlir][toy] Fix ConstantOp::parse in Ch2

[ggladilov]

Summary

parser.parseAttribute is called after parser.parseOptionalAttrDict and
by that moment attribute dictionary is already parsed, so it fails with
"error: expected attribute value".

It is not an option to leave parser.parseAttribute call only (remove
parser.parseOptionalAttrDict), because in this case it will parse
attribute dictionary as a whole, so resulting attribute would be
mlir::DictionaryAttr vs expected mlir::DenseElementsAttr deduced from
the first argument and fail with "error: custom op 'toy.constant'
invalid kind of attribute specified".

Signed-off-by: Gladilov, Gleb <[e-mail]>

[ggladilov]

This comment was removed by ggladilov.

[ggladilov]

Get rid of FileCheck call, toy.func and CHECK as successful parsing is
enough to make sure bug is not reproducible again.

[rriddle]

mlir/test/Examples/Toy/Ch2/constant.mlir

3 ↗

(On Diff #503849)

In which situations is IR like this being created? The custom format of toy.constant should never place the value attribute in the dictionary, it should always be handled separately. The attribute dictionary handling is there to process other, non-explicitly specified, attributes.

[ggladilov]

mlir/test/Examples/Toy/Ch2/constant.mlir

3 ↗

(On Diff #503849)

In which situations is IR like this being created?

It was manually written by me. I was attempting to conduct roundtrip testing (to better understand print & parse logic - unfortunately it is not explained in details) when I wrote some simple Toy IR, emitted MLIR and read it back again, but MLIR I was using on the last step I, for some reason, wrote manually.

Now I retried the procedure, but using MLIR generated from reading Toy IR and see everything works as is (and does not work with my changes).

Thanks for your help reviewing, now understand a bit more ;)

[ggladilov]

Filed by mistake

mlir/examples/toy/Ch2/mlir/Dialect.cpp

 /// various punctuation, as well as attributes, operands, types, etc. Each of
 /// these methods returns a `ParseResult`. This class is a wrapper around
 /// `LogicalResult` that can be converted to a boolean `true` value on failure,
 /// or `false` on success. This allows for easily chaining together a set of
 /// parser rules. These rules are used to populate an `mlir::OperationState`
 /// similarly to the `build` methods described above.
 mlir::ParseResult ConstantOp::parse(mlir::OpAsmParser &parser,
                                     mlir::OperationState &result) {
-  mlir::DenseElementsAttr value;
-  if (parser.parseOptionalAttrDict(result.attributes) ||
-      parser.parseAttribute(value, "value", result.attributes))
-    return failure();
+  if (parser.parseOptionalAttrDict(result.attributes)) {
+    return mlir::failure();
+  }
+
+  auto attributes = result.attributes.getAttrs();
+  if (attributes.size() != 1) {
+    return mlir::failure();
+  }
+
+  auto attribute = attributes.front();
+  if (attribute.getName() != "value") {
+    return mlir::failure();
+  }
+
+  auto value = attribute.getValue().dyn_cast<mlir::DenseElementsAttr>();
+  if (!value) {
+    return mlir::failure();
+  }
 
   result.addTypes(value.getType());
-  return success();
+  return mlir::success();
 }
 
 /// The 'OpAsmPrinter' class is a stream that allows for formatting
 /// strings, attributes, operands, types, etc.
 void ConstantOp::print(mlir::OpAsmPrinter &printer) {
   printer << " ";
   printer.printOptionalAttrDict((*this)->getAttrs(), /*elidedAttrs=*/{"value"});
   printer << getValue();