This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add elementAttr to TypedArrayAttrBase.
ClosedPublic

Authored by abeakkas on Jan 28 2020, 2:06 PM.

Details

Summary

In code generators, one can automate the translation of typed ArrayAttrs if element attribute translators are already implemented. However, the type of the element attribute is lost at the construction of TypedArrayAttrBase. With this change one can inspect the element type and generate the translation logic automatically, which will reduce the code repetition.

Diff Detail

Event Timeline

abeakkas created this revision.Jan 28 2020, 2:06 PM
abeakkas added a reviewer: ftynse.EditedJan 28 2020, 2:07 PM

This is a followup commit to https://reviews.llvm.org/D72888 .

Unit tests: pass. 62196 tests passed, 0 failed and 815 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

Can you provide a test?

nicolasvasilache resigned from this revision.Jan 31 2020, 11:56 AM

Can you provide a test?

Can you give me a pointer as to how to test this? I searched for a similar test for baseAttr in the repo but couldn't find one. This is something that you would use while inspecting TableGen records. Should the test be a cpp file under mlir/test/lib/TableGen that inspects a td file?

@antiagainst : what is the best way to test that?

antiagainst added a comment.EditedFeb 4 2020, 7:39 AM

@antiagainst : what is the best way to test that?

Sorry for the late reply! Existing tests for TableGen are testing TableGen backends in tree. What @abeakkas has here is a field for an out-of-tree TableGen backend so I'm not sure that there is a good approach except for just dumping the record without invoking any backend and check the field is as expected.

abeakkas updated this revision to Diff 243233.Feb 7 2020, 10:36 AM

Adding test that checks the record dump for elementAttr field.

antiagainst requested changes to this revision.Feb 8 2020, 4:04 AM
antiagainst added inline comments.
mlir/test/mlir-tblgen/op-attribute.td
250

Can we have RECORD-LABEL to match SomeTypedArrayAttr before this? Otherwise it's too broad a range to match against.

This revision now requires changes to proceed.Feb 8 2020, 4:04 AM
abeakkas updated this revision to Diff 243957.Feb 11 2020, 12:27 PM

Added RECORD-LABEL to SomeTypedArrayAttr test.

abeakkas marked an inline comment as done.Feb 11 2020, 12:28 PM
antiagainst accepted this revision.Feb 11 2020, 3:16 PM
rriddle accepted this revision.Feb 12 2020, 2:55 PM
This revision is now accepted and ready to land.Feb 12 2020, 2:55 PM

I don't have commit access to the GitHub repo. Can someone land the change for me?

This revision was automatically updated to reflect the committed changes.