This is an archive of the discontinued LLVM Phabricator instance.

Make ops with StructAttr's actually verify `isa<TheStruct>`.
ClosedPublic

Authored by silvas on Apr 27 2020, 5:53 PM.

Details

Summary

Previously, they would only only verify isa<DictionaryAttr> on such attrs
which resulted in crashes down the line from code assuming that the
verifier was doing the more thorough check introduced in this patch.
The key change here is for StructAttr to use
CPred<"$_self.isa<" # name # ">()"> instead of isa<DictionaryAttr>.

To test this, introduce struct attrs to the test dialect. Previously,
StructAttr was only being tested by unittests/, which didn't verify how
StructAttr interacted with ODS.

Diff Detail

Event Timeline

silvas created this revision.Apr 27 2020, 5:53 PM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle accepted this revision.Apr 28 2020, 11:32 AM
rriddle added inline comments.
mlir/test/lib/Dialect/Test/TestDialect.cpp
483

Why is the namespace necessary here? I wouldn't expect it to be.

This revision is now accepted and ready to land.Apr 28 2020, 11:32 AM
silvas updated this revision to Diff 260756.Apr 28 2020, 1:48 PM
silvas marked an inline comment as done.

update

mlir/test/lib/Dialect/Test/TestDialect.cpp
483

Indeed, removed. I think some prior version of the patch required it.

This revision was automatically updated to reflect the committed changes.