[PATCH] D153050: [mlir][ArmSME] Insert intrinsics to enable/disable ZA

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski accepted this revision.
awarzynski added a comment.

Cheers for addressing my comments, LGTM!



================
Comment at: mlir/test/Dialect/ArmSME/enable-arm-za.mlir:1-3
+// RUN: mlir-opt %s -enable-arm-streaming -convert-vector-to-llvm="enable-arm-sme" | FileCheck %s
+// RUN: mlir-opt %s -enable-arm-streaming=enable-za -convert-vector-to-llvm="enable-arm-sme" | FileCheck %s -check-prefix=CHECK-ENABLE-ZA
+// RUN: mlir-opt %s -convert-vector-to-llvm="enable-arm-sme" | FileCheck %s -check-prefix=CHECK-NO-ARM-STREAMING
----------------
Personal opinion, feel free to ignore.

1. Custom prefix everywhere - this way you use the prefixes to "document" the test cases (e.g. "ENABLE-ZA" clearly suggests that in this case `ZA` is explicitly enabled)
2. Drop `CHECK` from custom prefixes. I think that most people know that these are `CHECK` ... prefixes. And shorter prefix means less noise/repetition.
3. Match the order of checks with the `RUN` lines (so, right now, it would be `CHECK`, then `CHECK-ENABLE-ZA` and `CHECK-NO-ARM-STREAMING`).

Like I said, person opinion, aka nit :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153050/new/

https://reviews.llvm.org/D153050