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

Bump LLVM to 289b17635958d986b74683c932df6b1d12f37b70. #8225

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mikeurbach
Copy link
Contributor

This picked up a few breaking changes.

llvm/llvm-project@327d6270

This added new required methods to CallOpInterface and CallableOpInterface. These methods are related to arrays of dictionary attributes for each argument and result, and are normally implemented upstream by simply defining the right arg_attrs and res_attrs arguments on the operations. This allows some of the common utilities upstream to work with the attributes.

However, we seem to have actually moved away from arrays of per-argument or per-result attributes in most cases, and handle this ourselves when we want to. I opted to stub out the methods in the tablegen definitions of our operations, rather than adding optional attributes and setting them to null in builders and builder callsites like was done upstream.

There were also some accompanying name changes to a couple helper functions, which we were able to simply apply without needing to add new optional attributes everywhere.

llvm/llvm-project@e84f6b6a

This updated several LLVM dialect helper functions related to lookupOrCreateFn to return FailureOrLLVM::LLVMFuncOp. In most cases, we simply check for the error case and return it or signal pass failure. There was one function in the SMT to Z3 conversion that assumes this always succeeds and in this function I made an assertion rather than changing the function to also potentially return failure.

llvm/llvm-project@f4e3b878

Looks like upstream is simply moving from undef to poison in many cases, so this required us to update one test case.

This picked up a few breaking changes.

llvm/llvm-project@327d6270

This added new required methods to CallOpInterface and
CallableOpInterface. These methods are related to arrays of dictionary
attributes for each argument and result, and are normally implemented
upstream by simply defining the right arg_attrs and res_attrs
arguments on the operations. This allows some of the common utilities
upstream to work with the attributes.

However, we seem to have actually moved away from arrays of
per-argument or per-result attributes in most cases, and handle this
ourselves when we want to. I opted to stub out the methods in the
tablegen definitions of our operations, rather than adding optional
attributes and setting them to null in builders and builder callsites
like was done upstream.

There were also some accompanying name changes to a couple helper
functions, which we were able to simply apply without needing to add
new optional attributes everywhere.

llvm/llvm-project@e84f6b6a

This updated several LLVM dialect helper functions related to
lookupOrCreateFn to return FailureOr<LLVM::LLVMFuncOp>. In most cases,
we simply check for the error case and return it or signal pass
failure. There was one function in the SMT to Z3 conversion that
assumes this always succeeds and in this function I made an assertion
rather than changing the function to also potentially return failure.

llvm/llvm-project@f4e3b878

Looks like upstream is simply moving from undef to poison in many
cases, so this required us to update one test case.
@mikeurbach
Copy link
Contributor Author

I also have a version of this where I handled llvm/llvm-project@327d627 by adding arg_attrs and res_attrs like they did here, for example: llvm/llvm-project@327d627#diff-55d842f400d430d940f6036970f462764765330d7a617750cd21e7d499b1c5da

But this seemed like a step back for us... I think we pretty actively moved away from the per-argument / per-result attributes the way upstream does it on our ModuleLikes at least. I don't know what we are currently gaining by using these CallOpInterface and CallableOpInterfaces, but in the past I remember that trying to shoehorn HWModuleOp into the old FunctionLike interface was a problem. It seems better to me to stub out the new required methods, since they explicitly say they can just return null. They're mainly for IR printing / parsing helpers from what I can tell, so I don't think we're missing anything by opting out. But I leave it to the various codeowners to implement these by defining the attribute arrays if they want in a follow up PR.

Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

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

Seems like there is one more use of mlir-cpu-runner that needs to be renamed. But otherwise LGTM! Completely agree with your approach about the arg/res attrs!

@fabianschuiki
Copy link
Contributor

Thanks for fixing all of these! 🥳 👍 We haven't really used arg/result attributes in the Arc dialect at all, so the stubs are perfect.

@dtzSiFive
Copy link
Contributor

Thanks for the great work and write-up! LGTM!

@mikeurbach
Copy link
Contributor Author

Will plan to merge this EOD today once CI is passing.

@mikeurbach mikeurbach requested a review from uenoku February 12, 2025 19:10
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.

SV/Sim change looks great to me!

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.

5 participants