Page MenuHomePhabricator

[MLIR] Add support for explicitly signed / unsigned integer constants
Needs RevisionPublic

Authored by andidr on Mar 13 2020, 8:25 AM.

Details

Summary

Currently, only signless integers are allowed as constants. This patch additionally allows for explicitly signed and explicitly unsigned integer constants.

Diff Detail

Event Timeline

andidr created this revision.Mar 13 2020, 8:25 AM
stephenneuendorffer added inline comments.
mlir/test/IR/parser.mlir
501

My understanding of the design is that i32 is signless, ui32 is unsigned, and si32 is signed... It seems like you're not testing the explicitly signed case?

rriddle requested changes to this revision.Mar 13 2020, 9:24 AM

Thanks for the patch! StandardOps is designed to use only signless integers, any deviation will require an RFC and justification as to why the change is desired. See https://mlir.llvm.org/docs/Rationale/#integer-signedness-semantics
Thanks again.

This revision now requires changes to proceed.Mar 13 2020, 9:24 AM
andidr added a comment.EditedMar 13 2020, 12:44 PM

It seems that I have misinterpreted https://reviews.llvm.org/D72533 as the introduction of explicitly signed / unsigned types to MLIR in general, including std. Thanks for your clarification!

The patch originates from work on a frontend for Tensor Expressions that includes unsigned types in the source language and that generates operations from std and linalg. The translation is implemenyed recursively with functions simply returning mlir::Values. D72533 seemed like a comfortable way to keep this scheme and to avoid modeling the sign externally. However, without support for explicitly signed data types for the operations from std, this means that either the operands need to be converted to signless integers or the operations need to be re-implemented outside of std with support for explicitly signed / unsigned operands. I am not aware of any way to convert the operands and re-implementation of the operations seems quite redundant.

I understand your motivations to keep signless integers in std wherever possible. Any hint on how to work around the above-mentioned issues is highly appreciated. Thanks!

andidr marked an inline comment as done.Mar 13 2020, 12:50 PM
andidr added inline comments.
mlir/test/IR/parser.mlir
501

Thanks for pointing this out. This is coherent with my understanding. I'll add test cases for signed integers should the decision to support only signless integers in std be revised.

It seems that I have misinterpreted https://reviews.llvm.org/D72533 as the introduction of explicitly signed / unsigned types to MLIR in general, including std. Thanks for your clarification!

The patch originates from work on a frontend for Tensor Expressions that includes unsigned types in the source language and that generates operations from std and linalg. The translation is implemenyed recursively with functions simply returning mlir::Values. D72533 seemed like a comfortable way to keep this scheme and to avoid modeling the sign externally. However, without support for explicitly signed data types for the operations from std, this means that either the operands need to be converted to signless integers or the operations need to be re-implemented outside of std with support for explicitly signed / unsigned operands. I am not aware of any way to convert the operands and re-implementation of the operations seems quite redundant.

I understand your motivations to keep signless integers in std wherever possible. Any hint on how to work around the above-mentioned issues is highly appreciated. Thanks!

I'd love to discuss more. To make it easier, could you post about what you are doing on https://llvm.discourse.group/c/llvm-project/mlir/?

I'd love to discuss more. To make it easier, could you post about what you are doing on https://llvm.discourse.group/c/llvm-project/mlir/?

Done: https://llvm.discourse.group/t/explicitly-signed-unsigned-integers-in-std/731. Thanks for the suggestion!

nicolasvasilache resigned from this revision.Oct 2 2020, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 1:25 AM