Skip to content

Commit eb16603

Browse files
jpienaarjpienaar
committed
[mlir] Expand default arg builders to wrapped attributes.
Previously only unwrapped values were considered for default values in builders, expand to Attributes given the change to populate defaults. To avoid overlap between wrapped and unwrapped, the starting index is incremented depending on if the smallest starting index for default valued args can be materialized in unwrapped form or not. Given the above limitation this is pretty small change. `optional` is unfortunately meant to also represent conditionally set which means we cannot allow nullptr for all Attributes marked optional at the moment. Reviewed By: Mogball Differential Revision: https://reviews.llvm.org/D140705
1 parent fc55068 commit eb16603

File tree

3 files changed

+100
-7
lines changed

3 files changed

+100
-7
lines changed

‎mlir/test/mlir-tblgen/op-attribute.td‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -412,7 +412,7 @@ def DefaultDictAttrOp : NS_Op<"default_dict_attr_op", []> {
412412
// DEF: attributes.append(attrNames[1], odsBuilder.getDictionaryAttr(getDefaultDictAttrs(odsBuilder)));
413413

414414
// DECL-LABEL: DefaultDictAttrOp declarations
415-
// DECL: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::DictionaryAttr empty, ::mlir::DictionaryAttr non_empty)
415+
// DECL: build(::mlir::OpBuilder &odsBuilder, ::mlir::OperationState &odsState, ::mlir::DictionaryAttr empty = nullptr, ::mlir::DictionaryAttr non_empty = nullptr)
416416

417417

418418
// Test derived type attr.

‎mlir/test/mlir-tblgen/op-default-builder.td‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// RUN: mlir-tblgen -gen-op-decls -I %S/../../include %s | FileCheck %s
2+
3+
include "mlir/IR/AttrTypeBase.td"
4+
include "mlir/IR/EnumAttr.td"
5+
include "mlir/IR/OpBase.td"
6+
7+
def Test_Dialect : Dialect {
8+
let name = "test";
9+
let cppNamespace = "foobar";
10+
}
11+
class NS_Op<string mnemonic, list<Trait> traits> :
12+
Op<Test_Dialect, mnemonic, traits>;
13+
14+
def SomeAttr : Attr<CPred<"some-condition">, "some attribute kind"> {
15+
let storageType = "some-attr-kind";
16+
let returnType = "some-return-type";
17+
let convertFromStorage = "$_self.some-convert-from-storage()";
18+
let constBuilderCall = "some-const-builder-call($_builder, $0)";
19+
}
20+
21+
def AOp : NS_Op<"a_op", []> {
22+
let arguments = (ins
23+
FloatLike:$lhs,
24+
SomeAttr:$aAttr,
25+
DefaultValuedAttr<SomeAttr, "4.2">:$bAttr,
26+
OptionalAttr<SomeAttr>:$cAttr,
27+
DefaultValuedOptionalAttr<SomeAttr, "7.2">:$dAttr
28+
);
29+
}
30+
31+
// CHECK-LABEL: AOp declarations
32+
// Note: `cAttr` below could be conditionally optional and so the generation is
33+
// currently conservative.
34+
// CHECK-DAG: ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
35+
// CHECK-DAG: ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
36+
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-attr-kind aAttr, some-attr-kind bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-attr-kind dAttr);
37+
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value lhs, some-return-type aAttr, some-return-type bAttr, /*optional*/some-attr-kind cAttr, /*optional*/some-return-type dAttr = 7.2);
38+
39+
def BOp : NS_Op<"b_op", []> {
40+
let arguments = (ins
41+
DefaultValuedAttr<SomeAttr, "6.2">:$aAttr,
42+
DefaultValuedAttr<SomeAttr, "4.2">:$bAttr
43+
);
44+
}
45+
46+
// Verify that non-overlapping builders created where all could be elided.
47+
// CHECK-LABEL: BOp declarations
48+
// CHECK-DAG: some-attr-kind aAttr, some-attr-kind bAttr = nullptr);
49+
// CHECK-DAG: some-return-type aAttr = 6.2, some-return-type bAttr = 4.2);
50+
// CHECK-DAG: ::mlir::TypeRange resultTypes, some-attr-kind aAttr, some-attr-kind bAttr = nullptr);
51+
// CHECK-DAG: ::mlir::TypeRange resultTypes, some-return-type aAttr = 6.2, some-return-type bAttr = 4.2);
52+
53+
def COp : NS_Op<"c_op", []> {
54+
let arguments = (ins
55+
FloatLike:$value,
56+
OptionalAttr<SymbolRefArrayAttr>:$ag,
57+
OptionalAttr<SymbolRefArrayAttr>:$as,
58+
OptionalAttr<SymbolRefArrayAttr>:$nos,
59+
OptionalAttr<I64Attr>:$al,
60+
UnitAttr:$vo,
61+
UnitAttr:$non
62+
);
63+
}
64+
65+
// CHECK-LABEL: COp declarations
66+
// Note: `al` below could be conditionally optional and so the generation is
67+
// currently conservative.
68+
// CHECK-DAG: ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/::mlir::UnitAttr vo, /*optional*/::mlir::UnitAttr non = nullptr);
69+
// CHECK-DAG: ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/bool vo = false, /*optional*/bool non = false);
70+
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/::mlir::UnitAttr vo, /*optional*/::mlir::UnitAttr non = nullptr);
71+
// CHECK-DAG: ::mlir::TypeRange resultTypes, ::mlir::Value value, /*optional*/::mlir::ArrayAttr ag, /*optional*/::mlir::ArrayAttr as, /*optional*/::mlir::ArrayAttr nos, /*optional*/::mlir::IntegerAttr al, /*optional*/bool vo = false, /*optional*/bool non = false);

‎mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2883,16 +2883,20 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> &paramList,
28832883
// Successors and variadic regions go at the end of the parameter list, so no
28842884
// default arguments are possible.
28852885
bool hasTrailingParams = op.getNumSuccessors() || op.getNumVariadicRegions();
2886-
if (attrParamKind == AttrParamKind::UnwrappedValue && !hasTrailingParams) {
2886+
if (!hasTrailingParams) {
28872887
// Calculate the start index from which we can attach default values in the
28882888
// builder declaration.
28892889
for (int i = op.getNumArgs() - 1; i >= 0; --i) {
28902890
auto *namedAttr =
28912891
llvm::dyn_cast_if_present<tblgen::NamedAttribute *>(op.getArg(i));
2892-
if (!namedAttr || !namedAttr->attr.hasDefaultValue())
2892+
if (!namedAttr)
28932893
break;
28942894

2895-
if (!canUseUnwrappedRawValue(namedAttr->attr))
2895+
Attribute attr = namedAttr->attr;
2896+
// TODO: Currently we can't differentiate between optional meaning do not
2897+
// verify/not always error if missing or optional meaning need not be
2898+
// specified in builder. Expand isOptional once we can differentiate.
2899+
if (!attr.hasDefaultValue() && !attr.isDerivedAttr())
28962900
break;
28972901

28982902
// Creating an APInt requires us to provide bitwidth, value, and
@@ -2907,6 +2911,21 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> &paramList,
29072911
defaultValuedAttrStartIndex = i;
29082912
}
29092913
}
2914+
// Avoid generating build methods that are ambiguous due to default values by
2915+
// requiring at least one attribute.
2916+
if (defaultValuedAttrStartIndex < op.getNumArgs()) {
2917+
// TODO: This should have been possible as a cast<NamedAttribute> but
2918+
// required template instantiations is not yet defined for the tblgen helper
2919+
// classes.
2920+
auto *namedAttr =
2921+
cast<NamedAttribute *>(op.getArg(defaultValuedAttrStartIndex));
2922+
Attribute attr = namedAttr->attr;
2923+
if ((attrParamKind == AttrParamKind::WrappedAttr &&
2924+
canUseUnwrappedRawValue(attr)) ||
2925+
(attrParamKind == AttrParamKind::UnwrappedValue &&
2926+
!canUseUnwrappedRawValue(attr)))
2927+
++defaultValuedAttrStartIndex;
2928+
}
29102929

29112930
/// Collect any inferred attributes.
29122931
for (const NamedTypeConstraint &operand : op.getOperands()) {
@@ -2959,9 +2978,12 @@ void OpEmitter::buildParamList(SmallVectorImpl<MethodParameter> &paramList,
29592978

29602979
// Attach default value if requested and possible.
29612980
std::string defaultValue;
2962-
if (attrParamKind == AttrParamKind::UnwrappedValue &&
2963-
i >= defaultValuedAttrStartIndex) {
2964-
defaultValue += attr.getDefaultValue();
2981+
if (i >= defaultValuedAttrStartIndex) {
2982+
if (attrParamKind == AttrParamKind::UnwrappedValue &&
2983+
canUseUnwrappedRawValue(attr))
2984+
defaultValue += attr.getDefaultValue();
2985+
else
2986+
defaultValue += "nullptr";
29652987
}
29662988
paramList.emplace_back(type, namedAttr.name, StringRef(defaultValue),
29672989
attr.isOptional());

0 commit comments

Comments
 (0)