From d4381b3f93a6e53e1b35232b9a0039b1f5e04c6a Mon Sep 17 00:00:00 2001 From: River Riddle Date: Tue, 26 Apr 2022 11:21:18 -0700 Subject: [PATCH] [mlir:PDL] Fix a syntax ambiguity in pdl.attribute pdl.attribute currently has a syntax ambiguity that leads to the incorrect parsing of pdl.attribute operations with locations that don't also have a constant value. For example: ``` pdl.attribute loc("foo") ``` The above IR is treated as being a pdl.attribute with a constant value containing the location, `loc("foo")`, which is incorrect. This commit changes the syntax to use `= ` to clearly distinguish when the constant value is present, as opposed to just trying to parse an attribute. Differential Revision: https://reviews.llvm.org/D124582 --- mlir/include/mlir/Dialect/PDL/IR/PDLOps.td | 4 ++-- .../pdl-to-pdl-interp-matcher.mlir | 4 ++-- .../pdl-to-pdl-interp-rewriter.mlir | 2 +- mlir/test/Dialect/PDL/invalid.mlir | 2 +- mlir/test/Dialect/PDL/ops.mlir | 19 ++++++++++++++++--- .../Dialect/PDL/CPU/multiroot.mlir | 14 +++++++------- mlir/test/mlir-pdll/CodeGen/MLIR/expr.pdll | 2 +- mlir/test/python/dialects/pdl_ops.py | 2 +- 8 files changed, 31 insertions(+), 18 deletions(-) diff --git a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td index 4e43b2677f4a..dfd8318b8088 100644 --- a/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td +++ b/mlir/include/mlir/Dialect/PDL/IR/PDLOps.td @@ -118,14 +118,14 @@ def PDL_AttributeOp : PDL_Op<"attribute"> { %attr = pdl.attribute : %type // Define an attribute with a constant value: - %attr = pdl.attribute "hello" + %attr = pdl.attribute = "hello" ``` }]; let arguments = (ins Optional:$type, OptionalAttr:$value); let results = (outs PDL_Attribute:$attr); - let assemblyFormat = "(`:` $type^)? ($value^)? attr-dict-with-keyword"; + let assemblyFormat = "(`:` $type^)? (`=` $value^)? attr-dict-with-keyword"; let builders = [ OpBuilder<(ins CArg<"Value", "Value()">:$type), [{ diff --git a/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-matcher.mlir b/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-matcher.mlir index 457042767130..b94451c4a086 100644 --- a/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-matcher.mlir +++ b/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-matcher.mlir @@ -49,7 +49,7 @@ module @attributes { // CHECK-DAG: pdl_interp.check_type %[[ATTR1_TYPE]] is i64 pdl.pattern : benefit(1) { %type = type : i64 - %attr = attribute 10 : i64 + %attr = attribute = 10 : i64 %attr1 = attribute : %type %root = operation {"attr" = %attr, "attr1" = %attr1} rewrite %root with "rewriter" @@ -583,7 +583,7 @@ module @attribute_literal { // Check the correct lowering of an attribute that hasn't been bound. pdl.pattern : benefit(1) { - %attr = attribute 10 + %attr = attribute = 10 pdl.apply_native_constraint "constraint"(%attr: !pdl.attribute) %root = operation diff --git a/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-rewriter.mlir b/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-rewriter.mlir index 4d6d524e89fc..9f3141aade1d 100644 --- a/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-rewriter.mlir +++ b/mlir/test/Conversion/PDLToPDLInterp/pdl-to-pdl-interp-rewriter.mlir @@ -42,7 +42,7 @@ module @operation_attributes { %attr = attribute %root = operation "foo.op" {"attr" = %attr} rewrite %root { - %attr1 = attribute true + %attr1 = attribute = true %newOp = operation "foo.op" {"attr" = %attr, "attr1" = %attr1} erase %root } diff --git a/mlir/test/Dialect/PDL/invalid.mlir b/mlir/test/Dialect/PDL/invalid.mlir index a39d1c5e80f8..09d4467dcd88 100644 --- a/mlir/test/Dialect/PDL/invalid.mlir +++ b/mlir/test/Dialect/PDL/invalid.mlir @@ -36,7 +36,7 @@ pdl.pattern : benefit(1) { %type = type // expected-error@below {{expected only one of [`type`, `value`] to be set}} - %attr = attribute : %type 10 + %attr = attribute : %type = 10 %op = operation "foo.op" {"attr" = %attr} -> (%type : !pdl.type) rewrite %op with "rewriter" diff --git a/mlir/test/Dialect/PDL/ops.mlir b/mlir/test/Dialect/PDL/ops.mlir index 472dd250feaa..c15f53d4500d 100644 --- a/mlir/test/Dialect/PDL/ops.mlir +++ b/mlir/test/Dialect/PDL/ops.mlir @@ -1,6 +1,5 @@ // RUN: mlir-opt -split-input-file %s | mlir-opt -// Verify the generic form can be parsed. -// RUN: mlir-opt -split-input-file -mlir-print-op-generic %s | mlir-opt +// RUN: mlir-opt -split-input-file -mlir-print-op-generic -mlir-print-local-scope %s | FileCheck %s --check-prefix=CHECK-GENERIC // ----- @@ -138,7 +137,21 @@ pdl.pattern @apply_rewrite_with_no_results : benefit(1) { pdl.pattern @attribute_with_dict : benefit(1) { %root = operation rewrite %root { - %attr = attribute {some_unit_attr} attributes {pdl.special_attribute} + %attr = attribute = {some_unit_attr} attributes {pdl.special_attribute} apply_native_rewrite "NativeRewrite"(%attr : !pdl.attribute) } } + +// ----- + +// Check that we don't treat the trailing location of a pdl.attribute as the +// attribute value. + +pdl.pattern @attribute_with_loc : benefit(1) { + // CHECK-GENERIC: "pdl.attribute" + // CHECK-GENERIC-NOT: value = loc + %attr = attribute loc("bar") + + %root = operation {"attribute" = %attr} + rewrite %root with "rewriter" +} diff --git a/mlir/test/Integration/Dialect/PDL/CPU/multiroot.mlir b/mlir/test/Integration/Dialect/PDL/CPU/multiroot.mlir index e5b768302b97..eb04ceb1ce45 100644 --- a/mlir/test/Integration/Dialect/PDL/CPU/multiroot.mlir +++ b/mlir/test/Integration/Dialect/PDL/CPU/multiroot.mlir @@ -15,7 +15,7 @@ module @patterns { %rxact = pdl.operand : %in_type %weight = pdl.operand : %weight_type - %attr0 = pdl.attribute false + %attr0 = pdl.attribute = false %op0 = pdl.operation "tf.MatMul" (%rxact, %weight : !pdl.value, !pdl.value) {"transpose_a" = %attr0, "transpose_b" = %attr0} -> (%out_type : !pdl.type) pdl.rewrite %op0 { @@ -35,8 +35,8 @@ module @patterns { %rxdelta = pdl.operand : %out_type %weight = pdl.operand : %weight_type - %attr0 = pdl.attribute true - %attr1 = pdl.attribute false + %attr0 = pdl.attribute = true + %attr1 = pdl.attribute = false %op0 = pdl.operation "tf.MatMul" (%rxact, %rxdelta : !pdl.value, !pdl.value) {"transpose_a" = %attr0, "transpose_b" = %attr1} -> (%weight_type : !pdl.type) %val0 = pdl.result 0 of %op0 %op1 = pdl.operation "tf.Const" -> (%const_type : !pdl.type) @@ -156,7 +156,7 @@ module @patterns { %weight = pdl.operand : %weight_type %bias = pdl.operand : %bias_type - %attr0 = pdl.attribute false + %attr0 = pdl.attribute = false %op0 = pdl.operation "tf.MatMul" (%rxact, %weight : !pdl.value, !pdl.value) {"transpose_a" = %attr0, "transpose_b" = %attr0} -> (%out_type : !pdl.type) %val0 = pdl.result 0 of %op0 %op1 = pdl.operation "tf.BiasAdd" (%val0, %bias : !pdl.value, !pdl.value) -> (%out_type : !pdl.type) @@ -165,7 +165,7 @@ module @patterns { %val2 = pdl.result 0 of %op2 %op3 = pdl.operation "tf.ReluGrad" (%rxdelta, %val2 : !pdl.value, !pdl.value) -> (%out_type : !pdl.type) %val3 = pdl.result 0 of %op3 - %attr1 = pdl.attribute true + %attr1 = pdl.attribute = true %op4 = pdl.operation "tf.MatMul" (%rxact, %val3 : !pdl.value, !pdl.value) {"transpose_a" = %attr1, "transpose_b" = %attr0} -> (%weight_type : !pdl.type) %val4 = pdl.result 0 of %op4 %op5 = pdl.operation "kernel.GD" (%weight, %val4 : !pdl.value, !pdl.value) -> (%weight_type : !pdl.type) @@ -198,9 +198,9 @@ module @patterns { %rxdelta = pdl.operand : %out_type %weight = pdl.operand : %weight_type - %attr0 = pdl.attribute false + %attr0 = pdl.attribute = false %op0 = pdl.operation "tf.MatMul" (%rxact, %weight : !pdl.value, !pdl.value) {"transpose_a" = %attr0, "transpose_b" = %attr0} -> (%out_type : !pdl.type) - %attr1 = pdl.attribute true + %attr1 = pdl.attribute = true %op1 = pdl.operation "tf.MatMul" (%rxdelta, %weight : !pdl.value, !pdl.value) {"transpose_a" = %attr0, "transpose_b" = %attr1} -> (%in_type : !pdl.type) %op2 = pdl.operation "tf.MatMul" (%rxact, %rxdelta : !pdl.value, !pdl.value) {"transpose_a" = %attr1, "transpose_b" = %attr0} -> (%weight_type : !pdl.type) %val2 = pdl.result 0 of %op2 diff --git a/mlir/test/mlir-pdll/CodeGen/MLIR/expr.pdll b/mlir/test/mlir-pdll/CodeGen/MLIR/expr.pdll index e8db46c1dd3d..2d6960294537 100644 --- a/mlir/test/mlir-pdll/CodeGen/MLIR/expr.pdll +++ b/mlir/test/mlir-pdll/CodeGen/MLIR/expr.pdll @@ -5,7 +5,7 @@ //===----------------------------------------------------------------------===// // CHECK: pdl.pattern @AttrExpr -// CHECK: %[[ATTR:.*]] = attribute 10 +// CHECK: %[[ATTR:.*]] = attribute = 10 // CHECK: operation({{.*}}) {"attr" = %[[ATTR]]} Pattern AttrExpr => erase op<> { attr = attr<"10"> }; diff --git a/mlir/test/python/dialects/pdl_ops.py b/mlir/test/python/dialects/pdl_ops.py index b575f19bb6c4..3d9cd1927899 100644 --- a/mlir/test/python/dialects/pdl_ops.py +++ b/mlir/test/python/dialects/pdl_ops.py @@ -220,7 +220,7 @@ def test_native_rewrite(): # CHECK: pdl.pattern @attribute_with_value : benefit(1) { # CHECK: %0 = operation # CHECK: rewrite %0 { -# CHECK: %1 = attribute "value" +# CHECK: %1 = attribute = "value" # CHECK: apply_native_rewrite "NativeRewrite"(%1 : !pdl.attribute) # CHECK: } # CHECK: }