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

[Handshake] Fix canonicalizer not going through rewriter for RAUW. #7052

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dtzSiFive
Copy link
Contributor

Detected by -DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON.

cc #7047.

Detected by -DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON.

cc llvm#7047.
Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

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

Is modifyOpInPlace not a valid thing to use anymore? It's generally used for good reason. There may be some subtlety here that necessitates it.

@dtzSiFive
Copy link
Contributor Author

Is modifyOpInPlace not a valid thing to use anymore? It's generally used for good reason. There may be some subtlety here that necessitates it.

Thanks for taking a critical look here!

The RAUW's are updating ops using parentForkOp results, not that op itself, which is what I think was going wrong and addressed here. Possibly could keep it and also use rewriter for the RAUW from within? 🤔

@teqdruid
Copy link
Contributor

I'm not sure. Did Morten write this code? If so, the conservative thing to do is to wait for him to come back from vacation and get his review. Should be back in two or three weeks, but this doesn't seem urgent... yes?

@dtzSiFive
Copy link
Contributor Author

I'm not sure. Did Morten write this code? If so, the conservative thing to do is to wait for him to come back from vacation and get his review. Should be back in two or three weeks, but this doesn't seem urgent... yes?

Not urgent AFAIK. Sounds good to me, thanks!

@Dinistro
Copy link
Contributor

I don't think that there was any particular reason for this, but it might be still sensible to wait for Morten to confirm.

@mortbopet
Copy link
Contributor

I can come up with no good arguemnt for why modifyOpInPlace was used, instead of notifying things through the rewriter. Tests pass, so LGTM - Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants