Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Dialect] Traits and type parser/printer fixes to unblock downstream integrations #15

Merged

Conversation

sjain-stanford
Copy link
Collaborator

@sjain-stanford sjain-stanford commented Oct 26, 2023

  • Remove ParentOneOf trait from tcp group ops
    • No longer a constraint; fixes error: 'tcp.isolated_group' op expects parent op to be one of 'func.func, tcp.group, tcp.isolated_group'
  • Use default type parser/printer for TcpDialect
    • Used downstream; fixes error: dialect 'tcp' provides no type parsing hook
  • Add Tcp_IndexArrayType definition
    • Fixes ld.lld: error: undefined symbol: mlir::tcp::TcpDialect::printType(mlir::Type, mlir::DialectAsmPrinter&) const referenced by TcpDialect.cpp
  • Minor: rename pass registration

@sjain-stanford sjain-stanford changed the title [NFC] Rename pass registration Fixes from downstream integrations (remove ParentOneOf trait from tcp group ops, add default type parser) Oct 26, 2023
@sjain-stanford sjain-stanford changed the title Fixes from downstream integrations (remove ParentOneOf trait from tcp group ops, add default type parser) Fixes from downstream integrations Oct 26, 2023
Copy link
Collaborator

@navahgar navahgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 18 to 25
//===----------------------------------------------------------------------===//
// Tcp Type Definitions.
// Tcp Type Definitions
//===----------------------------------------------------------------------===//

def Tcp_IndexArrayType : Tcp_Type<"IndexArray", "index_array"> {
let summary = "IndexArray TCP type, to holds a list of index builtin type to represent shape";
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor nit: It might be better to have the basic types, Tcp_Scalar, Tcp_Tensor before we define IndexArrayType. The same would apply to the Tcp_QuantizedType as well. So, may be we can do it later.

Copy link
Collaborator Author

@sjain-stanford sjain-stanford Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tcp_Scalar depends on Tcp_QuantizedInt so it has to follow quantized types. However I've moved Tcp_IndexArrayType to the end.

@sjain-stanford sjain-stanford changed the title Fixes from downstream integrations [Dialect] Traits and type parser/printer fixes to unblock downstream integrations Oct 26, 2023
@sjain-stanford sjain-stanford merged commit 1808051 into cruise-automation:main Oct 26, 2023
1 check passed
@sjain-stanford sjain-stanford deleted the sambhav/nit_naming branch October 26, 2023 23:43
srinathava pushed a commit to srinathava/mlir-tcp that referenced this pull request Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants