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

[Arith] MLIR PresburgerSet compile fix mlir >= 160 #15638

Merged
merged 1 commit into from
Aug 31, 2023
Merged

[Arith] MLIR PresburgerSet compile fix mlir >= 160 #15638

merged 1 commit into from
Aug 31, 2023

Conversation

cbalint13
Copy link
Contributor

@cbalint13 cbalint13 commented Aug 28, 2023

Hi folks,

Some fixes for MLIR based analyzer module introduced by #14690 .


  • Make CMake at par with LLVM info:
{...}
-- Use llvm-config=llvm-config-64
-- LLVM libdir: /usr/lib64
-- Found MLIR
-- Build with MLIR
-- Set TVM_MLIR_VERSION=160
-- Found LLVM_INCLUDE_DIRS=/usr/include
{...}
--    USE_MKL                            : OFF
--    USE_MLIR                           : ON
--    USE_MSVC_MT                        : OFF
{...}
  • Fix several compilation errors:
error: cannot convert 'llvm::SmallVector<long int>' to 'llvm::ArrayRef<mlir::presburger::MPInt>'
error: no matching function for call to 'tvm::IntImm::IntImm(tvm::runtime::DataType, mlir::presburger::MPInt&)'
note:   no known conversion for argument 2 from 'mlir::presburger::MPInt' to 'int64_t' {aka 'long int'}

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

Copy link
Member

@junrushao junrushao left a 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?

@cbalint13
Copy link
Contributor Author

cbalint13 commented Aug 28, 2023

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?

@junrushao ,

Me either, not expert on this topic, unsure how #14690 passed it's own test (can't see related CI logs).
Since here we have compilation fixes (no changes), the #14690 test case included should be sufficient.


Let's wait the tests to pass in green, reported issue here might be related to:
(a) a different mlir = 16 version on my side
(b) a newer gcc 13 not willing to infer casts (unplausible)

If (a) will adjust this PR with some little #if version to keep pace with MLIR API changes (mlir >= 15, as expected).

Let's also ask authors: @multiverstack-intellif , @wrongtest-intellif

@cbalint13 cbalint13 changed the title [Arith] MLIR PresburgerSet compilation fix [Arith] MLIR PresburgerSet compile fix mlir >= 160 Aug 29, 2023
@cbalint13 cbalint13 requested a review from junrushao August 29, 2023 10:25
@cbalint13
Copy link
Contributor Author

cbalint13 commented Aug 29, 2023

If (a) will adjust this PR with some little #if version to keep pace with MLIR API changes (mlir >= 15, as expected).
Let's also ask authors: @multiverstack-intellif , @wrongtest-intellif

PR is now updated to be consistent with all mlir >= 15 releases.
Tested for: llvm/mlir 15.0.7, llvm/mlir 16.0.6, llvm/mlir 17.0.0rc3.

@multiverstack-intellif
Copy link
Contributor

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?

@junrushao ,

Me either, not expert on this topic, unsure how #14690 passed it's own test (can't see related CI logs). Since here we have compilation fixes (no changes), the #14690 test case included should be sufficient.

Let's wait the tests to pass in green, reported issue here might be related to: (a) a different mlir = 16 version on my side (b) a newer gcc 13 not willing to infer casts (unplausible)

If (a) will adjust this PR with some little #if version to keep pace with MLIR API changes (mlir >= 15, as expected).

Let's also ask authors: @multiverstack-intellif , @wrongtest-intellif

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.

@junrushao
Copy link
Member

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.

@cbalint13
Copy link
Contributor Author

cbalint13 commented Aug 30, 2023

@multiverstack-intellif , @junrushao

Trying to tackle similar issue as described at interblock analisys I noticed it can't compile on newer mlir.
I think just like LLVM, MLIR will have frequent api changes across releases, so maintenance like this PR will be needed.


To refresh, this PR now covers (tested on ) llvm/mlir: 15.0.7, 16.0.6 and 17.0.0rc3.

@junrushao junrushao merged commit 022299b into apache:main Aug 31, 2023
@junrushao
Copy link
Member

Thanks @cbalint13! This is merged!

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.

3 participants