Page MenuHomePhabricator

[mlir][SystemZ] Fix incompatible datalayout in SystemZ
ClosedPublic

Authored by imaihal on Mon, May 18, 8:37 AM.

Details

Summary

MLIR tests in "mlir/test/mlir-cpu-runner" fails in SystemZ (z14) because
of incompatible datalayout error. This patch fixes it by setting host
CPU name in createTargetMachine()

Diff Detail

Event Timeline

imaihal created this revision.Mon, May 18, 8:37 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 18, 8:38 AM

Without this patch, following tests fail in SystemZ(z14)

MLIR :: mlir-cpu-runner/linalg_integration_test.mlir
MLIR :: mlir-cpu-runner/sgemm_naive_codegen.mlir
MLIR :: mlir-cpu-runner/simple.mlir
MLIR :: mlir-cpu-runner/unranked_memref.mlir
MLIR :: mlir-cpu-runner/utils.mlir

Error message is as follows.

Failure value returned from cantFail wrapped call
Added modules have incompatible data layouts: E-m:e-i1:8:16-i8:8:16-i64:64-f128:64-a:8:16-n32:64 (module) vs E-m:e-i1:8:16-i8:8:16-i64:6\
4-f128:64-v128:64-a:8:16-n32:64 (jit)
ftynse requested changes to this revision.Mon, May 18, 9:11 AM
ftynse added a subscriber: ftynse.

Thanks! I only have a nit about variable naming.

mlir/lib/ExecutionEngine/ExecutionEngine.cpp
123–133

MLIR uses camelBack for variables, please change the names

This revision now requires changes to proceed.Mon, May 18, 9:11 AM
imaihal updated this revision to Diff 264769.Mon, May 18, 7:23 PM

[mlir][SystemZ] Fix incompatible datalayout in SystemZ

MLIR tests in "mlir/test/mlir-cpu-runner" fails in SystemZ (z14) because
of incompatible datalayout error. This patch fixes it by setting host
CPU name in createTargetMachine()

imaihal updated this revision to Diff 264770.Mon, May 18, 7:36 PM
  • [NFC] Replace MaybeAlign with Align in TargetTransformInfo.
  • [mlir][SystemZ] Fix incompatible datalayout in SystemZ
imaihal marked an inline comment as done.Mon, May 18, 7:41 PM

Thanks for the review! I changed the variable name to camelBack

imaihal added a comment.EditedMon, May 18, 7:59 PM

My wrong operation(submission) may add LLDB tag on this patch.

mehdi_amini accepted this revision.Mon, May 18, 11:11 PM
mehdi_amini added inline comments.
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
123–133

You should be able to write just std::string cpu = llvm::sys::getHostCPUName();? (Or std::string cpu(llvm::sys::getHostCPUName());)

ftynse accepted this revision.Tue, May 19, 1:04 AM
ftynse removed a project: Restricted Project.
This revision is now accepted and ready to land.Tue, May 19, 1:05 AM
imaihal updated this revision to Diff 265107.Tue, May 19, 6:10 PM

Changed getHostCPUName() part to reflect the comment.

imaihal marked 2 inline comments as done.Tue, May 19, 6:13 PM
imaihal added inline comments.
mlir/lib/ExecutionEngine/ExecutionEngine.cpp
123–133

I write std::string cpu(llvm::sys::getHostCPUName()); because std::string cpu = llvm::sys::getHostCPUName(); issued build error. Thanks for your review!

imaihal marked an inline comment as done.Tue, May 19, 6:41 PM

@ftynse and @mehdi_amini, Could you help to commit this if this patch ready for commit? I don't have write access to the repository.

This revision was automatically updated to reflect the committed changes.