This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add assertions to prevent adding null attributes
ClosedPublic

Authored by tpopp on Aug 23 2021, 11:26 AM.

Details

Summary

Setting attributes on operations results in an assertion failure, but
this could be missed currently by adding a null attribute to an
OperationState and building an Operation with that. This change adds
some additional assertions to make these different locations consistent
with each other.

Diff Detail

Event Timeline

tpopp created this revision.Aug 23 2021, 11:26 AM
tpopp requested review of this revision.Aug 23 2021, 11:26 AM
rriddle added inline comments.Aug 23 2021, 12:00 PM
mlir/include/mlir/IR/OperationSupport.h
486 ↗(On Diff #368159)

If we are going to assert when adding attributes, I would instead add asserts to NamedAttrList. Asserts there would catch more cases.

mlir/lib/IR/Operation.cpp
179 ↗(On Diff #368159)

This should be guaranteed on construction by DictionaryAttr. Is there a case that isn't being caught?

Thanks for the patch!

tpopp marked 2 inline comments as done.Aug 24 2021, 6:30 AM
tpopp added inline comments.
mlir/lib/IR/Operation.cpp
179 ↗(On Diff #368159)

I'm removing this one. The case that was not handled was appending to a NamedAttrList and then calling NamedAttrList::getDictionary(MLIRContext *context). That is handled by your suggestion above though, so I'll remove this one.

tpopp updated this revision to Diff 368330.Aug 24 2021, 6:31 AM
tpopp marked an inline comment as done.

Move the assertion logic to NamedAttrList.

rriddle accepted this revision.Aug 24 2021, 10:47 AM
rriddle added inline comments.
mlir/include/mlir/IR/OperationSupport.h
275 ↗(On Diff #368330)

Should we move this to push_back instead? (The method that this bottoms out to)

This revision is now accepted and ready to land.Aug 24 2021, 10:47 AM
tpopp updated this revision to Diff 368582.Aug 25 2021, 2:06 AM

Move assertions to NamedAttrList::push_back

tpopp marked an inline comment as done.Aug 25 2021, 2:07 AM
This revision was landed with ongoing or failed builds.Aug 25 2021, 2:08 AM
This revision was automatically updated to reflect the committed changes.