This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ods] Improve integer signedness modelling
ClosedPublic

Authored by antiagainst on Mar 4 2020, 7:25 AM.

Details

Summary

A previous commit added support for integer signedness in C++
IntegerType. This change introduces ODS definitions for
integer types and integer (element) attributes w.r.t. signedness.

This commit also updates various existing definitions' descriptions
to mention signless where suitable to make it more clear.

Positive and non-negative integer attributes are removed to avoid
the explosion of subclasses. Instead, one should use more atmoic
constraints together with Confined to model that. For example,
Confined<..., [IntPositive]>.

Diff Detail

Event Timeline

antiagainst created this revision.Mar 4 2020, 7:25 AM
Herald added 1 blocking reviewer(s): rriddle. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
rriddle accepted this revision.Mar 4 2020, 10:16 AM
rriddle marked an inline comment as done.

Thanks Lei!

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
569

IMO: we should cleanup all of these definitions to not use Arguments. It is much much cleaner.

mlir/include/mlir/IR/OpBase.td
381

nit: Move the signed before the unsigned.

859

nit: Move the signed before the unsigned.

This revision is now accepted and ready to land.Mar 4 2020, 10:16 AM
mravishankar resigned from this revision.Mar 4 2020, 10:19 AM

Just a minor comment on the SPIR-V side.

mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td
66

Do we want to used unsigned attribute here, i.e UI32Attr.

antiagainst marked 5 inline comments as done.

Reorder

antiagainst added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
569

I agree. This one is a bit extreme given we have many fields already. But I'll leave that to @ftynse to decide.

mlir/include/mlir/Dialect/SPIRV/SPIRVBase.td
66

Core EnumAttr and EnumAttrCase is still based on signless integers; this is to be consistent there given we are just simplifying the verifications for the known case comparison.

This revision was automatically updated to reflect the committed changes.