[Mlir-commits] [mlir] [mlir][ArmSME] Add a tests showing liveness issues in the tile allocator (PR #90447)

[Andrzej Warzyński]

================
@@ -0,0 +1,328 @@
+// RUN: mlir-opt %s -allocate-arm-sme-tiles -split-input-file -verify-diagnostics | FileCheck %s --check-prefix=CHECK-BAD
+
+// This file tests some aspects of liveness issues in the SME tile allocator.
+// These tests were designed with a new liveness-based tile allocator in mind,
+// with the current tile allocator these tests all give incorrect results (which
+// is documented by `CHECK-BAD`).
+//
+// Currently only the `CHECK-BAD` tests are run (as the new liveness based
+// allocator is not yet available -- so all other tests fail).
+
+//       CHECK-LIVE-RANGE: ========== Coalesced Live Ranges:
+//  CHECK-LIVE-RANGE-NEXT: @constant_with_multiple_users
+//       CHECK-LIVE-RANGE: ^bb0:
+//       CHECK-LIVE-RANGE: S  arm_sme.zero
+//  CHECK-LIVE-RANGE-NEXT: |S arm_sme.move_vector_to_tile_slice
+//  CHECK-LIVE-RANGE-NEXT: || arm_sme.move_vector_to_tile_slice
+//  CHECK-LIVE-RANGE-NEXT: |E test.some_use
+//  CHECK-LIVE-RANGE-NEXT: E  test.some_use
----------------
banach-space wrote:

IMHO, there's too much information here.

These comments do help understood the root cause leading to the incorrect result, yes. But only if you are familiar with the notation, which is not documented here. Also, the very identification of _what_ is wrong (rather than _why_) is quite tricky - that should be the key information conveyed here. IMHO, by adding more information to these tests that's unnecessarily obfuscated.

Also, the discussion of the root causes behind _why_ this code is wrong is already covered in:
* https://discourse.llvm.org/t/psa-armsme-lowering-pipeline-and-tile-allocation-changes/
* https://github.com/llvm/llvm-project/pull/90448

Separately, every patch should be self-contained. Instead of documenting the notation for live ranges, I'd much rather have these comments removed.

https://github.com/llvm/llvm-project/pull/90447