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

[LLVM] Add support to generate llvm.assume #14294

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

quic-sanirudh
Copy link
Contributor

We're adding support to generate llvm.assume from tir.assume as we currently see an error when lowering code with tir.assume in certain cases.

We're adding support to generate `llvm.assume` from `tir.assume` as we
currently see an error when lowering code with tir.assume in certain
cases.
@tvm-bot
Copy link
Collaborator

tvm-bot commented Mar 14, 2023

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

  • No users to tag found in teams: llvm See #10317 for details

Generated by tvm-bot

@quic-sanirudh
Copy link
Contributor Author

cc @Lunderberg @kparzysz-quic

There is a comment in layout_transformation.cc which suggests that tir.assume might be removed during lowering, but it seems that the tir.remove_assume pass has not yet been added to the default list of passes and so I ran into an error while lowering with assume.

Instead of adding this pass as a default one, I've added support to generate llvm.assume since this info might be useful for LLVM optimizations as well, and users can still manually add the tir.remove_assume pass before lowering to remove it.

Copy link
Contributor

@kparzysz-quic kparzysz-quic 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 good, but I'll defer to @Lunderberg to comment on what should happen to the assume builtins in TIR.

@quic-sanirudh
Copy link
Contributor Author

@Lunderberg Could you please take a look at this change and provide your inputs on handling tir.assume, thanks

Copy link
Contributor

@Lunderberg Lunderberg left a comment

Choose a reason for hiding this comment

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

The changes look good to me, and I like the idea of exposing it to the LLVM backend. The initial plan was to restrict the usage of tir.assume, such that it did not need to be supported by all code generators, but this is definitely a good use for it.

Perhaps there should be some description of builtins supported by each backend. That way, the RemoveAssume() pass could selectively remove the tir.assume() builtin from backends that do not support it, while preserving it for backends that do.

@kparzysz-quic kparzysz-quic merged commit 32e500b into apache:main Mar 16, 2023
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.

4 participants