-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[Arith] MLIR PresburgerSet compile fix mlir >= 160 #15638
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the bugfixes! Definitely not an expert here, and do you think we need to add a test for changes introduced in presurger_set.cc?
Me either, not expert on this topic, unsure how #14690 passed it's own test (can't see related CI logs). Let's wait the tests to pass in green, reported issue here might be related to: If (a) will adjust this PR with some little Let's also ask authors: @multiverstack-intellif , @wrongtest-intellif |
PR is now updated to be consistent with all |
I believe it's because of a), USE_MLIR is not turned ON in CI test, AFAIK, and only LLVM 14 & 15 is tested locally when #14690 is submitted. |
Sounds great, and it seems that we all agree with the changes being made! Will merge it in as soon as the CI is green. |
@multiverstack-intellif , @junrushao Trying to tackle similar issue as described at interblock analisys I noticed it can't compile on newer mlir. To refresh, this PR now covers (tested on ) llvm/mlir: |
Thanks @cbalint13! This is merged! |
Hi folks,
Some fixes for MLIR based analyzer module introduced by #14690 .
Tested using:
llvm/mlir 16.0.6
,llvm/mlir 15.0.7
,llvm/mlir 17.0.0rc3
Thanks,
~Cristian.
Cc @multiverstack-intellif , @wrongtest-intellif , @vinx13, @Hzfengsy , @junrushao , @tqchen