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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Thanks Cullen, just a couple of minor suggestions.



================
Comment at: mlir/lib/Dialect/ArmSME/Transforms/LegalizeForLLVMExport.cpp:57-58
+
+  // Enable ZA if the function has the 'arm_za' attribute and it hasn't already
+  // been enabled.
+  target.addDynamicallyLegalOp<func::FuncOp>([&](func::FuncOp funcOp) {
----------------
This comment suggests that `target.addDynamicallyLegalOp()` would enable `ZA`, but that's not quite true, is it? Instead, it marks `funcOp` as dynamically legal and provides a custom legality check:
* if `arm_za` attribute is present then legal if the first function Op is `arm_sme::aarch64_sme_za_enable`, 
* if `arm_za` is not present, mark as legal.

I guess that the comment refers to the overall legalisation process rather then specifically to this hook?


================
Comment at: mlir/test/Dialect/ArmSME/enable-arm-za.mlir:1
+// RUN: mlir-opt %s -enable-arm-streaming=enable-za -convert-vector-to-llvm="enable-arm-sme" | FileCheck %s
+
----------------
Could you add another RUN line without `enable-za` and verify that there's no `asm_sme.intr.za.{enable|disable}`?

It would also be good to test without the `-enable-arm-streaming` pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153050