This is an archive of the discontinued LLVM Phabricator instance.

Differential D118422

[mlir] Only build mlir-cpu-runner when the native arch is targeted
ClosedPublic

Authored by stella.stamenova on Jan 27 2022, 5:19 PM.

Details

Reviewers
mehdi_amini
Commits
rGc0861fcbb901: [mlir] Only build mlir-cpu-runner when the native arch is targeted
Summary

mlir-cpu-runner has a dependency on ExecutionEngine which is only built for the native arch. So currently mlir-cpu-runner does not link correctly when the native arch is not targeted.

Diff Detail

Event Timeline

stella.stamenova created this revision.Jan 27 2022, 5:19 PM
Herald added subscribers: sdasgup3, wenzhicui, wrengr and 21 others. · View Herald TranscriptJan 27 2022, 5:19 PM
stella.stamenova requested review of this revision.Jan 27 2022, 5:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2022, 5:19 PM
Herald added subscribers: stephenneuendorffer, nicolasvasilache. · View Herald Transcript
stella.stamenova added a reviewer: mehdi_amini.Jan 27 2022, 5:29 PM
Harbormaster completed remote builds in B146164: Diff 403843.Jan 27 2022, 5:51 PM
mehdi_amini accepted this revision.Jan 27 2022, 6:14 PM
mehdi_amini added inline comments.
mlir/tools/CMakeLists.txt
12

Should we detect the MLIRExecutionEngine target here instead?

This revision is now accepted and ready to land.Jan 27 2022, 6:14 PM
stella.stamenova added inline comments.Jan 27 2022, 6:49 PM
mlir/tools/CMakeLists.txt
12

We could. All the other parts of the project, such as the tests, pivot on the arch being present, so I made this check consistent, but either way makes sense. I would be tempted to change all the ones that use ExecutionEngine to check for the target if we made the change here.

mehdi_amini added inline comments.Jan 27 2022, 11:48 PM
mlir/tools/CMakeLists.txt
12

I guess either way is fine.

Closed by commit rGc0861fcbb901: [mlir] Only build mlir-cpu-runner when the native arch is targeted (authored by stella.stamenova). · Explain WhyJan 28 2022, 10:10 AM
This revision was automatically updated to reflect the committed changes.
stella.stamenova added a commit: rGc0861fcbb901: [mlir] Only build mlir-cpu-runner when the native arch is targeted.

Revision Contents

PathSize
mlir/
tools/
7 lines

Diff 404077

mlir/tools/CMakeLists.txt

add_subdirectory(mlir-cpu-runner)
add_subdirectory(mlir-lsp-server) add_subdirectory(mlir-lsp-server)
add_subdirectory(mlir-opt) add_subdirectory(mlir-opt)
add_subdirectory(mlir-pdll) add_subdirectory(mlir-pdll)
add_subdirectory(mlir-reduce) add_subdirectory(mlir-reduce)
add_subdirectory(mlir-shlib) add_subdirectory(mlir-shlib)
add_subdirectory(mlir-spirv-cpu-runner) add_subdirectory(mlir-spirv-cpu-runner)
add_subdirectory(mlir-translate) add_subdirectory(mlir-translate)
add_subdirectory(mlir-vulkan-runner) add_subdirectory(mlir-vulkan-runner)
# mlir-cpu-runner requires ExecutionEngine which is only built
# when the native target is configured in.
if(TARGET ${LLVM_NATIVE_ARCH})
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

Should we detect the MLIRExecutionEngine target here instead?

mehdi_amini: Should we detect the MLIRExecutionEngine target here instead?
stella.stamenovaAuthorUnsubmitted
Done
ReplyInline Actions

We could. All the other parts of the project, such as the tests, pivot on the arch being present, so I made this check consistent, but either way makes sense. I would be tempted to change all the ones that use ExecutionEngine to check for the target if we made the change here.

stella.stamenova: We could. All the other parts of the project, such as the tests, pivot on the arch being…
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

I guess either way is fine.

mehdi_amini: I guess either way is fine.
add_subdirectory(mlir-cpu-runner)
endif()