Page MenuHomePhabricator

[mlir][ods] Support default-valued attributes in optional groups
ClosedPublic

Authored by rkayaith on Sep 30 2022, 2:48 PM.

Details

Summary

Add support for default-valued attributes as optional-group anchors. The
attribute is considered present if it holds a non-default value.

Diff Detail

Event Timeline

rkayaith created this revision.Sep 30 2022, 2:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 2:48 PM
rkayaith added inline comments.Sep 30 2022, 6:24 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1261–1262

I didn't really understand the existing logic, it seemed odd to me to parse as optional under Normal context, but not under Optional context, but maybe I'm misunderstanding what "context" means here. I switched the use of GenContext so that genContext == Optional whenever the attribute should be optionally parsed (currently this is only for the first element of the then group).

rkayaith published this revision for review.Sep 30 2022, 6:24 PM
rkayaith added reviewers: rriddle, Mogball.
Mogball added inline comments.Oct 1 2022, 9:39 AM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1261–1262

GenContext::Optional means the element is inside an optional group, not whether the element should be optionally parsed.

rkayaith added inline comments.Oct 1 2022, 12:20 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1261–1262

I see, is there supposed to be a difference to how things are parsed in optional groups (other than the first element)? The only place I see genCtx used is this line, and it seems like it's allowing optional attributes to be optionally parsed when outside an optional group, but that just ends up breaking in the printer: https://github.com/llvm/llvm-project/issues/58064

Mogball added inline comments.Oct 13 2022, 7:01 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1261–1262

Not really. Only that certain elements are prohibited in optional groups (e.g. nested optional groups). The issue you linked just looks like a bug to me. I think the feature you are adding is useful but I'd prefer not to fiddle with the gen context

rkayaith updated this revision to Diff 467872.Oct 14 2022, 11:56 AM

preserve genCtx logic

Mogball accepted this revision.Oct 14 2022, 11:58 AM
Mogball added inline comments.
mlir/test/lib/Dialect/Test/TestOps.td
2178

there's a DefaultValuedStrAttr that lets you drop the quotes

This revision is now accepted and ready to land.Oct 14 2022, 11:58 AM
rkayaith added inline comments.Oct 14 2022, 12:00 PM
mlir/tools/mlir-tblgen/OpFormatGen.cpp
1096

on main genCtx ends up being ignored on this path, should this be parseAsOptional instead?

and similarly for the isOptional() check inside genEnumAttrParser

1261–1262

ok, updated the diff and tried to preserve existing genCtx logic

rkayaith updated this revision to Diff 467936.Oct 14 2022, 2:57 PM

fix default-valued symbol and enum attrs, use DefaultValuedStrAttr

mlir/tools/mlir-tblgen/OpFormatGen.cpp
1096

i switched these over to use parseAsOptional; otherwise default-valued symbols and enums break