This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add I1 support to DenseArrayAttr
ClosedPublic

Authored by Mogball on Aug 1 2022, 7:23 PM.

Details

Summary

This patch adds a DenseI1ArrayAttr to support arrays of i1. Importantly,
the implementation is as a simple ArrayRef<bool> instead of using bit
compression, which was problematic in DenseElementsAttr.

Diff Detail

Event Timeline

Mogball created this revision.Aug 1 2022, 7:23 PM
Mogball requested review of this revision.Aug 1 2022, 7:23 PM
rriddle added inline comments.Aug 1 2022, 7:37 PM
mlir/include/mlir/IR/BuiltinAttributes.h
808

For the resource version, we use DenseBoolResourceElementsAttr. We should try to keep these aligned.

mlir/test/IR/attribute.mlir
545

Do we intend to allow true or false values here?

rriddle added inline comments.Aug 1 2022, 7:40 PM
mlir/test/IR/attribute.mlir
545

If the API is really about bool, those would be more natural than raw integers; e.g. given that this is what DenseElementsAttr/ArrayAttr/etc. are all doing.

Mogball added inline comments.Aug 1 2022, 7:51 PM
mlir/test/IR/attribute.mlir
545

Would it make more sense for the syntax to be [:bool true, false, true]?

rriddle added inline comments.Aug 1 2022, 8:16 PM
mlir/test/IR/attribute.mlir
545

Nah, for bool as a type we always use i1. It's only values where we use the more ergonomic "true"/"false".

LG on the principle, with River's comments addressed.

Mogball updated this revision to Diff 449311.Aug 2 2022, 9:31 AM
Mogball marked 4 inline comments as done.

Change DenseI1ArrayAttr -> DenseBoolArrayAttr and change the values in the
assembly format to be true and false.

rriddle accepted this revision.Aug 2 2022, 12:18 PM
rriddle added inline comments.
mlir/lib/IR/BuiltinAttributes.cpp
813–815
842–846

Can we upgrade parseInteger to support bool?

This revision is now accepted and ready to land.Aug 2 2022, 12:18 PM

Also, you likely want to leave the bazel group to avoid getting hit on bazel failures (assuming you don't care about those anymore ;) )

Mogball updated this revision to Diff 449415.Aug 2 2022, 1:45 PM
Mogball marked 2 inline comments as done.

Add bool support to parseInteger

I think someone has to remove me from the group :P I wasn't able to remove myself!

Also PTAL

This revision was landed with ongoing or failed builds.Aug 4 2022, 7:25 AM
This revision was automatically updated to reflect the committed changes.
lattner added a subscriber: lattner.Aug 6 2022, 9:16 AM

+1, thank you for not doing bit compression!