Page MenuHomePhabricator

[MLIR] Disallow `sym_visibility`, `sym_name` and `type` attributes in the parsed attribute dictionary.
ClosedPublic

Authored by jurahul on Jan 6 2021, 3:17 PM.

Diff Detail

Event Timeline

jurahul created this revision.Jan 6 2021, 3:17 PM
jurahul requested review of this revision.Jan 6 2021, 3:17 PM
rriddle requested changes to this revision.Jan 6 2021, 3:19 PM
rriddle added inline comments.
mlir/include/mlir/IR/OpImplementation.h
528 ↗(On Diff #315009)

Why do you need any of this? Can't you just check for these after parsing?

This revision now requires changes to proceed.Jan 6 2021, 3:19 PM

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

I don't see why you can't just check these in FunctionImplementation.cpp. Why do you need to add more flags to the Parser methods?

This is in preparation of the visibility related changes discussed in https://llvm.discourse.group/t/external-function-declaration-has-changed-help-needed/2357. This change disallows attributes that sym_visibility/sym_name/type etc which are parsed/inferred from other places to be present in the attribute dictionary. This way, for the follow on changes, the function parser has just one parsed visibility value (parsed as keyword private/public/nested) to consider instead of trying to also look into the parsed attribute dictionary.

I don't see why you can't just check these in FunctionImplementation.cpp. Why do you need to add more flags to the Parser methods?

For this situation, having FunctionImplementation emit the error seems better given that "attribute is not allowed in attribute dictionary" does not provide much context on what the error is, or how it should be fixed. Can we provide a better error message instead?

jurahul updated this revision to Diff 315011.Jan 6 2021, 3:43 PM
jurahul marked an inline comment as done.

Simplify by moving check in parseFunctionLikeOp

rriddle added inline comments.Jan 6 2021, 3:49 PM
mlir/lib/IR/FunctionImplementation.cpp
211

Can you add a bit more context here?

Though maybe this is just me getting confused thinking about it. When I think of the attribute dictionary, I think of the complete dictionary on the operation and not just this specific one. We could provide specialized error messages for these attributes:

sym_visibility attribute should not be specified in the attribute dictionary, and instead be specified before the name of the function or something.

mlir/lib/Parser/AttributeParser.cpp
263 ↗(On Diff #315011)

Spurious change?

jurahul added inline comments.Jan 6 2021, 3:54 PM
mlir/lib/IR/FunctionImplementation.cpp
210

Still thinking of a clear error message: inferred attribute XXX is not allowed.. or implicit attribute XXX is not allowed?

jurahul added inline comments.Jan 6 2021, 3:56 PM
mlir/lib/IR/FunctionImplementation.cpp
211

Yes, when I say "attribute dictionary" I mean the parsed one, and not the complete one with parsed + inferred attributes. To be clear, we would need to specialize the error message per attribute. Let me try that.

rriddle accepted this revision.Jan 6 2021, 3:56 PM

LGTM after adding error messages. Thanks!

mlir/lib/IR/FunctionImplementation.cpp
210

Something like sounds good. XXX is an inferred attribute and should not appear in the explicit attribute dictionary.

This revision is now accepted and ready to land.Jan 6 2021, 3:56 PM
jurahul updated this revision to Diff 315018.Jan 6 2021, 4:26 PM

Better error messages

jurahul updated this revision to Diff 315022.Jan 6 2021, 4:33 PM

Make error message concise.