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

Upgrade LLVM to 7ac7d418ac2b16fd44789dcf48e2b5d73de3e715 and StableHLO to 2a0595b #2791

Merged
merged 8 commits into from
Apr 11, 2024
Merged

Upgrade LLVM to 7ac7d418ac2b16fd44789dcf48e2b5d73de3e715 and StableHLO to 2a0595b #2791

merged 8 commits into from
Apr 11, 2024

Conversation

hamptonm1
Copy link
Collaborator

@hamptonm1 hamptonm1 commented Apr 8, 2024

For the LLVM and StableHLO upgrade, I checked out commit number 7ac7d418ac2b16fd44789dcf48e2b5d73de3e715 and 2a0595b. One small update for LLVM is found here: llvm/llvm-project#85393.

Changing omp::WsLoopOp -> omp::WsloopOp

I also changed the pass in Sequence_with_dealloc.mlir from --buffer-deallocation-pipeline to --buffer-deallocation based on the following error I received:

+ /Users/meganhampton/zDLC/llvm-project/build/bin/FileCheck /Users/meganhampton/zDLC/onnx-mlir/test/mlir/conversion/onnx_to_krnl/Sequence/Sequence_with_dealloc.mlir
LLVM ERROR: checking for an interface (`mlir::bufferization::BufferDeallocationOpInterface`) that was promised by dialect 'arith' but never implemented. This is generally an indication that the dialect extension implementing the interface was never registered.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:

@hamptonm1 hamptonm1 self-assigned this Apr 8, 2024
@@ -1,4 +1,4 @@
// RUN: onnx-mlir-opt -O3 --shape-inference --convert-onnx-to-krnl --buffer-deallocation-pipeline %s -split-input-file | FileCheck %s
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tungld Do you know why this lit test had a pass named --buffer-deallocation-pipeline when I only see --buffer-deallocation pass defined... so I changed it back. Let me know your thoughts please if there is any other changes I should make. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this test is to check if memref.dealloc is inserted correctly or not. When you changed it back, I saw that memref.dealloc was missing.

I think --buffer-deallocation is the old mechanism, so we moved to the new one --buffer-deallocation-pipeline from the PR #2568.
It's not recommended to go back to the old mechanism since it will finally be removed in LLVM.

When I blamed this file, @chentong319 is the original author introducing this test. @chentong319 may have a better explanation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The sequence bufferization is a special case. You can do anything (including deleting this test) to make your PR pass. I will come back to fix it when onnx-mlir has moved to the new llvm level.

@hamptonm1
Copy link
Collaborator Author

@jenkins-droid test this please

1 similar comment
@hamptonm1
Copy link
Collaborator Author

@jenkins-droid test this please

@hamptonm1 hamptonm1 changed the title Upgrade LLVM to c511c90680ee and StableHLO to 1bdf7c2 Upgrade LLVM to 7ac7d418ac2b16fd44789dcf48e2b5d73de3e715 and StableHLO to 2a0595b Apr 9, 2024
@hamptonm1 hamptonm1 marked this pull request as ready for review April 9, 2024 20:45
@hamptonm1 hamptonm1 requested review from brnorris03 and tungld April 9, 2024 20:45
Copy link
Collaborator

@brnorris03 brnorris03 left a comment

Choose a reason for hiding this comment

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

Thank you for working on this update!

I noticed that StableHLO just updated to 1e6ce5e284f5, which is from yesterday -- it didn't seem like there were a lot of changes, maybe worth a try?

// RUN: onnx-mlir-opt -O3 --shape-inference --convert-onnx-to-krnl --buffer-deallocation-pipeline %s -split-input-file | FileCheck %s
// RUN: onnx-mlir-opt -O3 --shape-inference --convert-onnx-to-krnl --buffer-deallocation %s -split-input-file | FileCheck %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure if this is related, but buffer-deallocation-pipeline is supposed to be used after one-shot-bufferize.

@hamptonm1
Copy link
Collaborator Author

Thank you for working on this update!

I noticed that StableHLO just updated to 1e6ce5e284f5, which is from yesterday -- it didn't seem like there were a lot of changes, maybe worth a try?

Okay I can try and see how it goes! :)

@hamptonm1 hamptonm1 requested a review from chentong319 April 10, 2024 14:17
Copy link
Collaborator

@chentong319 chentong319 left a comment

Choose a reason for hiding this comment

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

LGTM!

@hamptonm1 hamptonm1 merged commit 5b1d90b into onnx:main Apr 11, 2024
6 of 7 checks passed
@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #14700 [push] Upgrade LLVM to 7ac7d418... started at 11:12

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #13695 [push] Upgrade LLVM to 7ac7d418... started at 11:22

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #14670 [push] Upgrade LLVM to 7ac7d418... started at 10:12

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #14670 [push] Upgrade LLVM to 7ac7d418... passed after 2 hr 9 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #14700 [push] Upgrade LLVM to 7ac7d418... passed after 2 hr 16 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #13695 [push] Upgrade LLVM to 7ac7d418... passed after 3 hr 3 min

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.

6 participants