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

[FIRRTL][LowerIntrinsics] Add stat and preserve if no changes. #6911

Merged
merged 1 commit into from
May 8, 2024

Conversation

dtzSiFive
Copy link
Contributor

No description provided.

@@ -87,8 +87,10 @@ class IntrinsicOpConversion final

IntrinsicOpConversion(MLIRContext *context,
const ConversionMapTy &conversions,
size_t &numConversions,
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 not sure if this is "safe" for a pattern to do?

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

This looks safe to me. As far as I see applyPartialConversion doesn't start a thread though they are implemented as therad-safe.

@dtzSiFive
Copy link
Contributor Author

This looks safe to me. As far as I see applyPartialConversion doesn't start a thread though they are implemented as therad-safe.

Thanks, @uenoku . That's my concern -- they sure look like something that's meant to be used concurrently in theory if nothing else.

My other doubt was that patterns themselves don't know if a thing was converted -- only that they reported at this point it could be (and used the rewriter to express what that means). So the number possibly could be off depending on the rewrite strategy / exploration, or more simply if ultimately conversion fails.

If this was functionally relevant that would be more concerning, but this should fail conservatively reliably for the latter and cause analyses to not be preserved which is what happens without this PR anyway.

And if this is a problem in the future or we regret it it's not a big deal to back out or just drop entirely.

The conversion framework DOES have the ability to let you track the set of operations converted but that seems kinda heavyweight for just counting. Maybe it is preferable though? 🤔 .

@dtzSiFive dtzSiFive merged commit 54ace0c into llvm:main May 8, 2024
4 checks passed
@dtzSiFive dtzSiFive deleted the feature/lower-intrinsics-analyses branch May 8, 2024 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants