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.

Sign up for GitHub

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

Jump to bottom

[NFC][Support] Factor out TypeConversionPattern into support lib #5055

Merged
merged 2 commits into from
Apr 18, 2023
Merged

[NFC][Support] Factor out TypeConversionPattern into support lib #5055

merged 2 commits into from
Apr 18, 2023
+135 −172

Conversation

teqdruid
Copy link
Contributor

This ConversionPattern is used in two places and is likely generally useful. Move it into the support lib.

Fixes #4899 as a side-effect.

@teqdruid teqdruid added ESI HWArith The `hwarith` dialect labels Apr 17, 2023
@teqdruid teqdruid requested a review from mortbopet April 17, 2023 20:13
@teqdruid teqdruid force-pushed the dev/jodemme/typeconversion-refactor branch from 15e0758 to ec22f39 Compare April 17, 2023 20:19
@teqdruid teqdruid force-pushed the dev/jodemme/typeconversion-refactor branch from ec22f39 to 227e535 Compare April 17, 2023 22:20
@mortbopet
Copy link
Contributor

mortbopet commented Apr 18, 2023 •
edited
Loading

Are you sure that this fixes the root cause for #4899? As written in the issue, I'm a little suspect of the msft.module builder, seeing as this pattern seems to work fine on all other (re. hw.module) container ops.

mortbopet
mortbopet reviewed Apr 18, 2023
View reviewed changes
lib/Support/ConversionPatterns.cpp
Region *newRegion = &newOp->getRegion(i);

// TypeConverter::SignatureConversion drops argument locations, so we need
// to manually copy them over (a verifier in e.g. HWModule checks this).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if we plan on upstreaming this, then (a verifier in e.g. HWModule checks this) seems unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m thinking before we try to upstream this, someone should fix the signature conversion in MLIR. There is a fixme in there.

@teqdruid
Copy link
Contributor Author

Are you sure that this fixes the root cause for #4899? As written in the issue, I'm a little suspect of the msft.module builder, seeing as this pattern seems to work fine on all other (re. hw.module) container ops

I'm not sure why it works differently in the hwarith lowering, but I definitely saw that same assertion on hw.module in the data window lowering pass.

@teqdruid
Copy link
Contributor Author

This ConversionPattern is used in two places and is likely generally useful. Move it into the support lib.

Fixes #4899 as a side-effect.

@teqdruid teqdruid merged commit a770c71 into main Apr 18, 2023
@teqdruid teqdruid deleted the dev/jodemme/typeconversion-refactor branch April 18, 2023 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mortbopet mortbopet mortbopet left review comments

Assignees
No one assigned
Labels
ESI HWArith The `hwarith` dialect
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MSFT] Assertion triggered if printing msft.module during pattern rewrite debugging
2 participants
@teqdruid @mortbopet