[mlir][ODS] Omit printing default-valued attributes in oilists #68880
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
mlir:core
|
@llvm/pr-subscribers-mlir-openmp @llvm/pr-subscribers-mlir-core Author: Benjamin Maxwell (MacDue) ChangesThis makes these match the behaviour of optional attributes (which are omitted when they are their default value of none). This allows for concise assembly formats without a custom printer. An extra print of " " is also removed, this does change any existing uses of oilists, but if the parameter before the oilist is optional, that would previously add an extra space. This #68694 + some fixes for the MLIR Python tests, unfortunately GitHub does not allow re-opening PRs 😕 Full diff: https://github.com/llvm/llvm-project/pull/68880.diff 8 Files Affected:
diff --git a/flang/test/Lower/OpenMP/FIR/atomic-read.f90 b/flang/test/Lower/OpenMP/FIR/atomic-read.f90
index ff2b651953f2abc..0079b347fac8de6 100644
--- a/flang/test/Lower/OpenMP/FIR/atomic-read.f90
+++ b/flang/test/Lower/OpenMP/FIR/atomic-read.f90
@@ -14,7 +14,7 @@
!CHECK: %[[VAR_X:.*]] = fir.alloca i32 {bindc_name = "x", uniq_name = "_QFEx"}
!CHECK: %[[VAR_Y:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"}
!CHECK: omp.atomic.read %[[VAR_X]] = %[[VAR_Y]] memory_order(acquire) hint(uncontended) : !fir.ref<i32>, i32
-!CHECK: omp.atomic.read %[[VAR_A]] = %[[VAR_B]] memory_order(relaxed) hint(none) : !fir.ref<!fir.char<1>>, !fir.char<1>
+!CHECK: omp.atomic.read %[[VAR_A]] = %[[VAR_B]] memory_order(relaxed) : !fir.ref<!fir.char<1>>, !fir.char<1>
!CHECK: omp.atomic.read %[[VAR_C]] = %[[VAR_D]] memory_order(seq_cst) hint(contended) : !fir.ref<!fir.logical<4>>, !fir.logical<4>
!CHECK: omp.atomic.read %[[VAR_E]] = %[[VAR_F]] hint(speculative) : !fir.ref<!fir.char<1,8>>, !fir.char<1,8>
!CHECK: omp.atomic.read %[[VAR_G]] = %[[VAR_H]] hint(nonspeculative) : !fir.ref<f32>, f32
diff --git a/flang/test/Lower/OpenMP/FIR/critical.f90 b/flang/test/Lower/OpenMP/FIR/critical.f90
index c6ac818fe21aa6e..b86729f8a98e370 100644
--- a/flang/test/Lower/OpenMP/FIR/critical.f90
+++ b/flang/test/Lower/OpenMP/FIR/critical.f90
@@ -2,7 +2,7 @@
!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s --check-prefix="OMPDialect"
!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | fir-opt --fir-to-llvm-ir | tco | FileCheck %s --check-prefix="LLVMIR"
-!OMPDialect: omp.critical.declare @help2 hint(none)
+!OMPDialect: omp.critical.declare @help2
!OMPDialect: omp.critical.declare @help1 hint(contended)
subroutine omp_critical()
diff --git a/flang/test/Lower/OpenMP/critical.f90 b/flang/test/Lower/OpenMP/critical.f90
index 9fbd172df96421c..5a4d2e4815df49e 100644
--- a/flang/test/Lower/OpenMP/critical.f90
+++ b/flang/test/Lower/OpenMP/critical.f90
@@ -1,6 +1,6 @@
!RUN: %flang_fc1 -emit-hlfir -fopenmp %s -o - | FileCheck %s
-!CHECK: omp.critical.declare @help2 hint(none)
+!CHECK: omp.critical.declare @help2
!CHECK: omp.critical.declare @help1 hint(contended)
subroutine omp_critical()
diff --git a/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir b/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
index 1df27dd9957e594..881d738b413ef15 100644
--- a/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
+++ b/mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
@@ -90,7 +90,7 @@ func.func @wsloop(%arg0: index, %arg1: index, %arg2: index, %arg3: index, %arg4:
// CHECK-LABEL: @atomic_write
// CHECK: (%[[ARG0:.*]]: !llvm.ptr<i32>)
// CHECK: %[[VAL0:.*]] = llvm.mlir.constant(1 : i32) : i32
-// CHECK: omp.atomic.write %[[ARG0]] = %[[VAL0]] hint(none) memory_order(relaxed) : !llvm.ptr<i32>, i32
+// CHECK: omp.atomic.write %[[ARG0]] = %[[VAL0]] memory_order(relaxed) : !llvm.ptr<i32>, i32
func.func @atomic_write(%a: !llvm.ptr<i32>) -> () {
%1 = arith.constant 1 : i32
omp.atomic.write %a = %1 hint(none) memory_order(relaxed) : !llvm.ptr<i32>, i32
@@ -474,4 +474,4 @@ llvm.func @_QPtarget_map_with_bounds(%arg0: !llvm.ptr<i32>, %arg1: !llvm.ptr<arr
omp.terminator
}
llvm.return
-}
\ No newline at end of file
+}
diff --git a/mlir/test/Dialect/OpenMP/ops.mlir b/mlir/test/Dialect/OpenMP/ops.mlir
index 13cbea6c9923c22..27c31824c0506df 100644
--- a/mlir/test/Dialect/OpenMP/ops.mlir
+++ b/mlir/test/Dialect/OpenMP/ops.mlir
@@ -831,7 +831,7 @@ omp.critical.declare @mutex6 hint(contended, nonspeculative)
omp.critical.declare @mutex7 hint(uncontended, speculative)
// CHECK: omp.critical.declare @mutex8 hint(contended, speculative)
omp.critical.declare @mutex8 hint(contended, speculative)
-// CHECK: omp.critical.declare @mutex9 hint(none)
+// CHECK: omp.critical.declare @mutex9
omp.critical.declare @mutex9 hint(none)
// CHECK: omp.critical.declare @mutex10
omp.critical.declare @mutex10
@@ -909,7 +909,7 @@ func.func @omp_atomic_read(%v: memref<i32>, %x: memref<i32>) {
omp.atomic.read %v = %x hint(nonspeculative, contended) : memref<i32>, i32
// CHECK: omp.atomic.read %[[v]] = %[[x]] memory_order(seq_cst) hint(contended, speculative) : memref<i32>, i32
omp.atomic.read %v = %x hint(speculative, contended) memory_order(seq_cst) : memref<i32>, i32
- // CHECK: omp.atomic.read %[[v]] = %[[x]] memory_order(seq_cst) hint(none) : memref<i32>, i32
+ // CHECK: omp.atomic.read %[[v]] = %[[x]] memory_order(seq_cst) : memref<i32>, i32
omp.atomic.read %v = %x hint(none) memory_order(seq_cst) : memref<i32>, i32
return
}
@@ -927,7 +927,7 @@ func.func @omp_atomic_write(%addr : memref<i32>, %val : i32) {
omp.atomic.write %addr = %val memory_order(relaxed) : memref<i32>, i32
// CHECK: omp.atomic.write %[[ADDR]] = %[[VAL]] hint(uncontended, speculative) : memref<i32>, i32
omp.atomic.write %addr = %val hint(speculative, uncontended) : memref<i32>, i32
- // CHECK: omp.atomic.write %[[ADDR]] = %[[VAL]] hint(none) : memref<i32>, i32
+ // CHECK: omp.atomic.write %[[ADDR]] = %[[VAL]] : memref<i32>, i32
omp.atomic.write %addr = %val hint(none) : memref<i32>, i32
return
}
@@ -1004,7 +1004,7 @@ func.func @omp_atomic_update(%x : memref<i32>, %expr : i32, %xBool : memref<i1>,
omp.yield(%const:i32)
}
- // CHECK: omp.atomic.update hint(none) %[[X]] : memref<i32>
+ // CHECK: omp.atomic.update %[[X]] : memref<i32>
// CHECK-NEXT: (%[[XVAL:.*]]: i32):
// CHECK-NEXT: %[[NEWVAL:.*]] = llvm.add %[[XVAL]], %[[EXPR]] : i32
// CHECK-NEXT: omp.yield(%[[NEWVAL]] : i32)
@@ -1181,7 +1181,7 @@ func.func @omp_atomic_capture(%v: memref<i32>, %x: memref<i32>, %expr: i32) {
omp.atomic.write %x = %expr : memref<i32>, i32
}
- // CHECK: omp.atomic.capture hint(none) {
+ // CHECK: omp.atomic.capture {
// CHECK-NEXT: omp.atomic.update %[[x]] : memref<i32>
// CHECK-NEXT: (%[[xval:.*]]: i32):
// CHECK-NEXT: %[[newval:.*]] = llvm.add %[[xval]], %[[expr]] : i32
diff --git a/mlir/test/python/dialects/transform_structured_ext.py b/mlir/test/python/dialects/transform_structured_ext.py
index 0f89d4137455a41..c9b7802e1cc4532 100644
--- a/mlir/test/python/dialects/transform_structured_ext.py
+++ b/mlir/test/python/dialects/transform_structured_ext.py
@@ -439,7 +439,7 @@ def testTileToForallCompact(target):
structured.TileUsingForallOp(matmul, num_threads=[2, 3, 4])
# CHECK-LABEL: TEST: testTileToForallCompact
# CHECK: = transform.structured.tile_using_forall
- # CHECK-SAME: num_threads [2, 3, 4] tile_sizes []
+ # CHECK-SAME: num_threads [2, 3, 4]
# CHECK-SAME: (!transform.op<"linalg.matmul">) -> (!transform.any_op, !transform.any_op)
@@ -454,7 +454,7 @@ def testTileToForallLoopsAndTileOpTypes(target):
)
# CHECK-LABEL: TEST: testTileToForallLoopsAndTileOpTypes
# CHECK: = transform.structured.tile_using_forall
- # CHECK-SAME: num_threads [2, 3, 4] tile_sizes []
+ # CHECK-SAME: num_threads [2, 3, 4]
# CHECK-SAME: (!transform.any_op) -> (!transform.op<"scf.forall">, !transform.op<"linalg.matmul">)
@@ -464,7 +464,7 @@ def testTileToForallTileSizes(target):
structured.TileUsingForallOp(target, tile_sizes=[2, 3, 4])
# CHECK-LABEL: TEST: testTileToForallTileSizes
# CHECK: = transform.structured.tile_using_forall
- # CHECK-SAME: num_threads [] tile_sizes [2, 3, 4]
+ # CHECK-SAME: tile_sizes [2, 3, 4]
@run
diff --git a/mlir/test/python/dialects/transform_vector_ext.py b/mlir/test/python/dialects/transform_vector_ext.py
index 1a0a9e1d6ecbde7..a51f2154d1f7d54 100644
--- a/mlir/test/python/dialects/transform_vector_ext.py
+++ b/mlir/test/python/dialects/transform_vector_ext.py
@@ -94,30 +94,24 @@ def enum_configurable_patterns():
)
# CHECK: transform.apply_patterns.vector.lower_transpose
- # CHECK-SAME: lowering_strategy = eltwise
- # CHECK-SAME: avx2_lowering_strategy = false
vector.ApplyLowerTransposePatternsOp()
# CHECK: transform.apply_patterns.vector.lower_transpose
- # CHECK-SAME: lowering_strategy = eltwise
- # CHECK-SAME: avx2_lowering_strategy = false
+ # This is the default strategy, not printed.
vector.ApplyLowerTransposePatternsOp(
lowering_strategy=vector.VectorTransposeLowering.EltWise
)
# CHECK: transform.apply_patterns.vector.lower_transpose
# CHECK-SAME: lowering_strategy = flat_transpose
- # CHECK-SAME: avx2_lowering_strategy = false
vector.ApplyLowerTransposePatternsOp(
lowering_strategy=vector.VectorTransposeLowering.Flat
)
# CHECK: transform.apply_patterns.vector.lower_transpose
# CHECK-SAME: lowering_strategy = shuffle_1d
- # CHECK-SAME: avx2_lowering_strategy = false
vector.ApplyLowerTransposePatternsOp(
lowering_strategy=vector.VectorTransposeLowering.Shuffle1D
)
# CHECK: transform.apply_patterns.vector.lower_transpose
# CHECK-SAME: lowering_strategy = shuffle_16x16
- # CHECK-SAME: avx2_lowering_strategy = false
vector.ApplyLowerTransposePatternsOp(
lowering_strategy=vector.VectorTransposeLowering.Shuffle16x16
)
diff --git a/mlir/tools/mlir-tblgen/OpFormatGen.cpp b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
index bdb97866a47fc9d..18ca34379a71a0e 100644
--- a/mlir/tools/mlir-tblgen/OpFormatGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpFormatGen.cpp
@@ -2009,6 +2009,16 @@ static void genEnumAttrPrinter(const NamedAttribute *var, const Operator &op,
" }\n";
}
+/// Generate a check that a DefaultValuedAttr has a value that is non-default.
+static void genNonDefaultValueCheck(MethodBody &body, const Operator &op,
+ AttributeVariable &attrElement) {
+ FmtContext fctx;
+ Attribute attr = attrElement.getVar()->attr;
+ fctx.withBuilder("::mlir::OpBuilder((*this)->getContext())");
+ body << " && " << op.getGetterName(attrElement.getVar()->name) << "Attr() != "
+ << tgfmt(attr.getConstBuilderTemplate(), &fctx, attr.getDefaultValue());
+}
+
/// Generate the check for the anchor of an optional group.
static void genOptionalGroupPrinterAnchor(FormatElement *anchor,
const Operator &op,
@@ -2042,12 +2052,7 @@ static void genOptionalGroupPrinterAnchor(FormatElement *anchor,
if (attr.hasDefaultValue()) {
// Consider a default-valued attribute as present if it's not the
// default value.
- FmtContext fctx;
- fctx.withBuilder("::mlir::OpBuilder((*this)->getContext())");
- body << " && " << op.getGetterName(element->getVar()->name)
- << "Attr() != "
- << tgfmt(attr.getConstBuilderTemplate(), &fctx,
- attr.getDefaultValue());
+ genNonDefaultValueCheck(body, op, *element);
return;
}
llvm_unreachable("attribute must be optional or default-valued");
@@ -2158,7 +2163,6 @@ void OperationFormat::genElementPrinter(FormatElement *element,
// Emit the OIList
if (auto *oilist = dyn_cast<OIListElement>(element)) {
- genLiteralPrinter(" ", body, shouldEmitSpace, lastWasPunctuation);
for (auto clause : oilist->getClauses()) {
LiteralElement *lelement = std::get<0>(clause);
ArrayRef<FormatElement *> pelement = std::get<1>(clause);
@@ -2170,8 +2174,14 @@ void OperationFormat::genElementPrinter(FormatElement *element,
for (VariableElement *var : vars) {
TypeSwitch<FormatElement *>(var)
.Case([&](AttributeVariable *attrEle) {
- body << " || " << op.getGetterName(attrEle->getVar()->name)
+ body << " || (" << op.getGetterName(attrEle->getVar()->name)
<< "Attr()";
+ Attribute attr = attrEle->getVar()->attr;
+ if (attr.hasDefaultValue()) {
+ // Don't print default-valued attributes.
+ genNonDefaultValueCheck(body, op, *attrEle);
+ }
+ body << ")";
})
.Case([&](OperandVariable *ele) {
if (ele->getVar()->isVariadic()) {
|
This makes these match the behaviour of optional attributes (which are omitted when they are their default value of `none`). This allows for concise assembly formats without a custom printer. An extra print of " " is also removed, this does change any existing uses of oilists, but if the parameter before the oilist is optional, that would previously add an extra space.
joker-eph
approved these changes
Oct 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes these match the behaviour of optional attributes (which are omitted when they are their default value of none). This allows for concise assembly formats without a custom printer.
An extra print of " " is also removed, this does change any existing uses of oilists, but if the parameter before the oilist is optional, that would previously add an extra space.
This #68694 + some fixes for the MLIR Python tests, unfortunately GitHub does not allow re-opening PRs 😕