This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Port the remaining integration tests to use SVE
ClosedPublic

Authored by awarzynski on Feb 7 2023, 9:49 AM.

Details

Summary

This patch updates the remaining SparseCompiler integration tests to
target SVE when available.

TODO: Triage sparse_matmul.mlir and sparse_tanh.mlir

[1] https://github.com/llvm/llvm-project/issues/58531

Diff Detail

Event Timeline

awarzynski created this revision.Feb 7 2023, 9:49 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
awarzynski requested review of this revision.Feb 7 2023, 9:49 AM
aartbik added inline comments.Feb 7 2023, 12:12 PM
mlir/test/Integration/Dialect/SparseTensor/CPU/concatenate.mlir
18

We actually always run this right? If so, we should really make this (here and everywhere)

// Do the same run, but now with direct IR generation and, if available, VLA

Matt added a subscriber: Matt.Feb 7 2023, 8:32 PM
awarzynski updated this revision to Diff 495828.Feb 8 2023, 6:33 AM
  • Updated comments (as per suggestion from @aartbik)
  • Fixed options that are passed to mlir-opt (with these changes most tests pass)

There are currently two tests that are XFAIL'ed - I will triage them shortly.

Tested with:

  • cmake -DMLIR_INCLUDE_INTEGRATION_TESTS=On -DMLIR_RUN_ARM_SVE_TESTS=On <other cmake flags>
  • cmake -DMLIR_INCLUDE_INTEGRATION_TESTS=On -DMLIR_RUN_ARM_SVE_TESTS=On -DARM_EMULATOR_EXECUTABLE="qemu-aarch64" -DARM_EMULATOR_LLI_EXECUTABLE=<build-dir>/bin/lli <other cmake flags>
awarzynski updated this revision to Diff 495839.Feb 8 2023, 7:18 AM

Split "sparse_reduce_custom.mlir" into two tests

I've split sparse_reduce_custom.mlir into:

  • "sparse_reduce_custom.mlir", and
  • "sparse_reduce_custom_prod.mlir".

The latter contains product reducitons that are currently supported on AArch64. We did the same for
"sparse_reductions.mlir" in https://reviews.llvm.org/D121304.

awarzynski updated this revision to Diff 495841.Feb 8 2023, 7:20 AM

Add the missing test file

awarzynski edited the summary of this revision. (Show Details)Feb 8 2023, 7:21 AM
awarzynski updated this revision to Diff 495842.Feb 8 2023, 7:23 AM

Actually adds the file, sorry for the noise.

aartbik accepted this revision.Feb 8 2023, 12:38 PM

Note that in the long run it would perhaps be desirable to really skip the run if there is no VLA available, since we now do another similar run in most cases, which wastes testing resources.
But it is okay to proceed for now. Perhaps add one TODO in the lit setup for this, so we remember.

This revision is now accepted and ready to land.Feb 8 2023, 12:38 PM

Thanks for reviewing!

Note that in the long run it would perhaps be desirable to really skip the run if there is no VLA available, since we now do another similar run in most cases, which wastes testing resources.
But it is okay to proceed for now.

I have also thought about it and created https://github.com/llvm/llvm-project/issues/60628 to track this.

Btw, I created https://github.com/llvm/llvm-project/issues/60626 for the outstanding issues with the generated output.

frgossen added inline comments.
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_out_mult_elt.mlir
20

The enable-runtime-library=false option causes the values to be slightly different in our tests.
Instead of (14, 20, 0, 0) the values are only close to 0 (14, 20, -1.21979e-12, -1.21979e-12).
Any idea why?

frgossen added inline comments.Feb 9 2023, 11:33 AM
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_out_reduction.mlir
20

We run into a similar issue here. Here, it is ( 7, 69, -1414812757, -1414812757 ) when ( 7, 69, 0, 0 ) is expected

Peiming added inline comments.
mlir/test/Integration/Dialect/SparseTensor/CPU/sparse_out_mult_elt.mlir
20

Probably because we allocated more than needed (because it is impossible to know the nnz beforehand), and values after "20" are uninitialized random value, this should not cause problem as we would access it.

You can try out enable-buffer-initialization=true to force sparse compiler to zero out newly allocated memory and see.

20

Probably because we allocated more than needed (because it is impossible to know the nnz beforehand), and values after "20" are uninitialized random value, this should not cause problem as we would access it.

Sorry, *would not

@frgossen , I'm sorry that you are hitting these issues :(

The enable-runtime-library=false option causes the values to be slightly different in our tests. Instead of (14, 20, 0, 0) the values are only close to 0 (14, 20, -1.21979e-12, -1.21979e-12). Any idea why?

No, sorry. But perhaps we should be using fpcmp for verifying FP output 🤔 .

clang-aarch64-sve-vla (which runs these tests) is green, so ideally I would avoid reverting this change. Perhaps you could blacklist the failing tests and create a GitHub ticket like https://github.com/llvm/llvm-project/issues/60626? We may need to approach this on a case-by-case basis.

Fix in https://reviews.llvm.org/D143674

(our new push_back uses size/capacity a bit different, so a vector_transfer+print may see unitialized memory, which is why the enable-buffer-initialization was introduced to make sure memory is consistent)

if there are issues beyond this problem, please let me know!