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.

Sign up for GitHub

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

Jump to bottom

Update LLVM/MHLO to week of 2022/11/14 #1905

Merged
merged 8 commits into from
Dec 13, 2022
Merged

Update LLVM/MHLO to week of 2022/11/14 #1905

merged 8 commits into from
Dec 13, 2022

Conversation

gargaroff
Copy link
Collaborator

@gargaroff gargaroff commented Dec 7, 2022 •
edited
Loading

Green LLVM commit: e864ac694540342d5e59f59c525c5082f2594fb8
Green MHLO commit: eab364ba2a66bd0613efb94f8a738c1c97aaee92

The commits listed above are the green commits from Nov. 14.

This is the LLVM bump that @ljfitz mentioned in #1624. We would love to kick off a discussion about introducing a regular rotation for bumping to green commits, similar to what torch-mlir is doing.

Changelog:

mlir-hlo changed their directory structure, requiring two include_directories directives to make it buildable.
Generation of #map has changed, requiring large NFC changes to test references.

Signed-off-by: Dominik Montada dominik.montada@amd.com

@gargaroff
Copy link
Collaborator Author

I hereby confirm that I abide by the DCO requirements.

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@gargaroff
feat(onnx-mlir): bump to llvm commit 'e864ac694540342d5e59f59c525c508…
1267ad5 
…2f2594fb8'

Signed-off-by: Dominik Montada <dominik.montada@amd.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

gargaroff
gargaroff commented Dec 7, 2022
View reviewed changes
test/mlir/krnl/krnl_enable_memory_pool.mlir
@@ -47,7 +47,7 @@ func.func @test_allocs_not_lowered(%arg0: memref<10x10xf32>, %arg1: memref<10x10
return %0 : memref<10x10xf32>
}

// CHECK: [[MAP:#.+]] = affine_map<(d0, d1)
// CHECK: [[MAP:#.+]] = affine_map<(d0, d1) -> (0, d1 floordiv 64, 0, d0 floordiv 32, d0 mod 32, d1 mod 64)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The previous version only matched on the prefix of the map and it happened to work. Now with the update to affine maps, only matching the prefix will match the wrong map. I had to make the CHECK more specific to match the correct map

@sstamenova
Copy link
Collaborator

@jenkins-droid test this please

@tungld
Copy link
Collaborator

tungld commented Dec 8, 2022

@gargaroff Jenkins amd64, ppc64le, and s390x failed due to lit tests for NNPA. I guess they failed because of map-related stuffs also:

Failed Tests (5):
  Open Neural Network Frontend :: accelerators/nnpa/conversion/zhigh-to-zlow/gru.mlir
  Open Neural Network Frontend :: accelerators/nnpa/conversion/zhigh-to-zlow/lstm.mlir
  Open Neural Network Frontend :: accelerators/nnpa/conversion/zhigh-to-zlow/stick-unstick.mlir
  Open Neural Network Frontend :: accelerators/nnpa/conversion/zhigh-to-zlow/stickified-constant.mlir
  Open Neural Network Frontend :: accelerators/nnpa/conversion/zhigh-to-zlow/test-datalayout.mlir

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@gargaroff
Copy link
Collaborator Author

@tungld Thanks for pointing me to them. I didn't realize that I was building without accelerators. Locally it's passing now, so I hope this one works now.

@gargaroff
test(accelerators): update references for nnpa
62a9dbc 
Signed-off-by: Dominik Montada <dominik.montada@amd.com>
@gargaroff
Merge branch 'main' into dominik.bump_to_llvm_nov_14
fb6ebe9 
Signed-off-by: Dominik Montada <dominik.montada@amd.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld
Copy link
Collaborator

tungld commented Dec 8, 2022

@jenkins-droid test this please

tungld
tungld approved these changes Dec 8, 2022
View reviewed changes
Copy link
Collaborator

@tungld tungld left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the update!

@gargaroff
Copy link
Collaborator Author

You're welcome and thanks for the review!

sstamenova
sstamenova reviewed Dec 8, 2022
View reviewed changes
CMakeLists.txt Outdated
@@ -151,6 +151,10 @@ add_subdirectory(third_party/rapidcheck)

if (ONNX_MLIR_ENABLE_MHLO)
add_subdirectory(third_party/mlir-hlo EXCLUDE_FROM_ALL)

# mlir-hlo depends on these paths being visible, but does not propagate them.
include_directories(${CMAKE_CURRENT_SOURCE_DIR}/third_party/mlir-hlo)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd use ONNX_MLIR_SRC_ROOT and ONNX_MLIR_BIN_ROOT. Is there an issue opened against mlir-hlo to propagate the paths going forward?

Copy link
Collaborator Author

@gargaroff gargaroff Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have opened tensorflow/mlir-hlo#52, but so far there hasn't been any real reaction to it.

Also I have updated CMake to use the two variables you mentioned instead.

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@gargaroff
build(CMake): use ONNX_MLIR_SRC_ROOT, ONNX_MLIR_BIN_ROOT for mlir-hlo…
aa9b0ed 
… includes

Signed-off-by: Dominik Montada <dominik.montada@amd.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld tungld merged commit 3e76df0 into onnx:main Dec 13, 2022
@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #8981 [push] Update LLVM/MHLO to week... started at 20:23

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #8997 [push] Update LLVM/MHLO to week... started at 21:23

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #8047 [push] Update LLVM/MHLO to week... started at 21:27

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #8981 [push] Update LLVM/MHLO to week... passed after 2 hr 6 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #8047 [push] Update LLVM/MHLO to week... passed after 2 hr 37 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #8997 [push] Update LLVM/MHLO to week... passed after 2 hr 41 min

@gargaroff gargaroff deleted the dominik.bump_to_llvm_nov_14 branch March 20, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sstamenova sstamenova sstamenova approved these changes

@tungld tungld tungld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gargaroff @jenkins-droid @sstamenova @tungld